archive mirror
 help / color / mirror / Atom feed
From: Dominique Martinet <>
To: Doron Roberts-Kedes <>
Cc: Tom Herbert <>,
	Dave Watson <>,
	"David S. Miller" <>,,
Subject: Re: [PATCH] strparser: remove any offset before parsing messages
Date: Wed, 22 Aug 2018 00:51:13 +0200	[thread overview]
Message-ID: <20180821225113.GA6515@nautica> (raw)
In-Reply-To: <>

Doron Roberts-Kedes wrote on Tue, Aug 21, 2018:
> On Tue, Aug 21, 2018 at 09:36:55PM +0200, Dominique Martinet wrote:
> > One of the solutions I had suggested was adding a flag at strparser
> > setup time to only do that pull for users which cannot handle offset,
> > but nobody seemed interested two weeks ago. I can still do that.
> This seems overly complicated. 

That sounds much simpler than adding a similar feature to bpf if it
doesn't already support it -- we already have an user-provided struct at
strparser init timer (strp->cb) in which it would be easy to add a flag
saying whether the callbacks support offset or not.

That's maybe three more lines than the current patch, which is also
pretty simple, I'm not sure what you're expecting from alternative
solutions to call that overly complicated...

> It seems like we mainly agree that the proposed patch is suboptimal for
> existing clients of the library that use offset correctly (tls). 
> It also seems like you've identified that the proper fix is in bpf. 

I don't think bpf itself needs to be changed here -- the offset is
stored in a strparser specific struct so short of such a skb_pull I
think we'd need to change the type of the bpf function, pass it it the
extra parameter, and make it a user visible change breaking the kcm
API... And I have no idea for sockmap but probably something similar.

I can't think of that as better than adding a flag to strparser.

(Also, note that pskb_pull will not copy any data or allocate memory
unless we're pulling past the end of the skb, which seems pretty
unlikely in that situation as we should have consumed any fully "eaten"
skb before getting to a new one here -- so in practice this patch just
adds a skb->data += offset with safety guards "just in case")

> As an aside, I would recommend reaching the netdev FAQ page:
> It contains helpful hints about how to format email subjects (specifying
> net vs. net-next) and determining when trees are closed (currently
> closed). 

Thank you for pointing out to that document.
While this bug has been around from day one of kcm it is still a bug fix
so I'd rather let maintainers decide which tree it goes to.

I wasn't aware that net-next closes during the Linus merge window, but
if you want to split hairs, the FAQ says "do not send new net-next
content to netdev [while closed]" and this isn't new content as the v0
of the patch was mostly the same and sent before the 4.18 release, (and,
ironically, did not get any comment).
Anyway, sure, I'll wait until next week for a v2 if that matters.


  reply	other threads:[~2018-08-21 22:51 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-09 22:40 [PATCH v0] strparser: remove any offset before parsing messages Dominique Martinet
2018-08-21 12:51 ` [PATCH] " Dominique Martinet
2018-08-21 14:53   ` Doron Roberts-Kedes
2018-08-21 19:36     ` Dominique Martinet
2018-08-21 21:15       ` Doron Roberts-Kedes
2018-08-21 22:51         ` Dominique Martinet [this message]
2018-08-21 23:35           ` Doron Roberts-Kedes
2018-08-22  0:46             ` Dominique Martinet
2018-08-22  2:33               ` Doron Roberts-Kedes
2018-08-22  5:47                 ` Dominique Martinet
2018-08-22 18:38                   ` Dave Watson
2018-08-23  1:04                     ` Dominique Martinet

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:

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

  git send-email \
    --in-reply-to=20180821225113.GA6515@nautica \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).