LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Dominique Martinet <asmadeus@codewreck.org>
To: Tom Herbert <tom@herbertland.com>
Cc: David Miller <davem@davemloft.net>,
	doronrk@fb.com, Tom Herbert <tom@quantonium.net>,
	Dave Watson <davejwatson@fb.com>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] kcm: remove any offset before parsing messages
Date: Fri, 15 Feb 2019 02:57:05 +0100	[thread overview]
Message-ID: <20190215015705.GA17974@nautica> (raw)
In-Reply-To: <CALx6S35HBKgtYkFde_AhCMPRtZfcVYfCYf=mLKjWSLLAfhMHyQ@mail.gmail.com>

Tom Herbert wrote on Thu, Feb 14, 2019:
> > The best alternative I see is adding a proper helper to get
> > "kcm_rx_msg(skb)->offset" from bpf and document it so users aren't as
> > lost as I have been; I'm not quite sure how/where to add such a helper
> > though as I've barely looked at the bpf code until now, but should we go
> > for that?
> 
> I'd rather not complicate the bpf code for this. Can we just always do
> an pskb_pull after skb_clone?

Which skb_clone are you thinking of?
If you're referring to the one in strparser, that was my very first
approach here[1], but Dordon shot it down saying that this is not an
strparser bug but a kcm bug since there are ways for users to properly
get the offset and use it -- and the ktls code does it right.

Frankly, my opinion still is that it'd be better in strparser because
there also is some bad use in bpf sockmap (they never look at the offset
either, while kcm uses it for rcv but not for parse), but looking at it
from an optimal performance point of view I agree the user can make
better decision than strparser so I understand where he comes from.


This second patch[2] (the current thread) now does an extra clone if
there is an offset, but the problem really isn't in the clone but the
pull itself that can fail and return NULL when there is memory pressure.
For some reason I hadn't been able to reproduce that behaviour with
strparser doing the pull, but I assume it would also be possible to hit
in extreme situations, I'm not sure...

So anyway, we basically have three choices that I can see:
 - push harder on strparser and go back to my first patch ; it's simple
and makes using strparser easier/safer but has a small overhead for
ktls, which uses the current strparser implementation correctly.
I hadn't been able to get this to fail with KASAN last time I tried but
I assume it should still be possible somehow.

 - the current patch, that I could only get to fail with KASAN, but does
complexify kcm a bit; this also does not fix bpf sockmap at all.
Would still require to fix the hang, so make strparser retry a few times
if strp->cb.parse_msg failed maybe? Or at least get the error back to
userspace somehow.

 - my last suggestion to document the kcm behaviour, and if possible add
a bpf helper to make proper usage easier ; this will make kcm harder to
use on end users but it's better than not being documented and seeing
random unappropriate lengths being parsed.



[1] first strparser patch
http://lkml.kernel.org/r/1534855906-22870-1-git-send-email-asmadeus@codewreck.org
[2] current thread's patch
http://lkml.kernel.org/r/1536657703-27577-1-git-send-email-asmadeus@codewreck.org


Thanks,
-- 
Dominique

  reply	other threads:[~2019-02-15  1:57 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-11  9:21 Dominique Martinet
2018-09-12  5:36 ` Dominique Martinet
2018-09-18  1:45   ` David Miller
2018-09-18  1:57     ` Dominique Martinet
2018-09-18  2:40       ` David Miller
2018-09-18  2:45         ` Dominique Martinet
2018-09-18  2:51           ` David Miller
2018-09-18  2:58             ` Dominique Martinet
2018-10-31  2:56       ` Dominique Martinet
2019-02-15  1:00         ` Dominique Martinet
2019-02-15  1:20           ` Tom Herbert
2019-02-15  1:57             ` Dominique Martinet [this message]
2019-02-15  2:48               ` Tom Herbert
2019-02-15  3:31                 ` Dominique Martinet
2019-02-15  4:01                   ` Tom Herbert
2019-02-15  4:52                     ` Dominique Martinet
2019-02-20  4:11                       ` Dominique Martinet
2019-02-20 16:18                         ` Tom Herbert
2019-02-21  8:22                           ` Dominique Martinet
2019-02-22 19:24                             ` Tom Herbert
2019-02-22 20:27                               ` Dominique Martinet
2019-02-22 21:01                                 ` Tom Herbert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190215015705.GA17974@nautica \
    --to=asmadeus@codewreck.org \
    --cc=davejwatson@fb.com \
    --cc=davem@davemloft.net \
    --cc=doronrk@fb.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=tom@herbertland.com \
    --cc=tom@quantonium.net \
    --subject='Re: [PATCH v2] kcm: remove any offset before parsing messages' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).