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 04:31:02 +0100	[thread overview]
Message-ID: <20190215033102.GA3099@nautica> (raw)
In-Reply-To: <CALx6S37MZadJ=PaAd+SSv9hxSX9kFTmTUtijPGA39JCx3PYq1Q@mail.gmail.com>

Tom Herbert wrote on Thu, Feb 14, 2019:
> > 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...
>
> This option looks the best to me, at least as a way to fix the issue
> without requiring a change to the API. If the pull fails, doesn't that
> just mean that the parser fails? Is there some behavior with this
> patch that is not being handled gracefully?

Yes, the parser fails with -ENOMEM ; that is not handled gracefully at
all: from a user point of view, the connection just hangs (recvmsg never
returns), without so much as a message in dmesg either.

It took me a while to figure out what failed exactly as I did indeed
expect strparser/kcm to handle that better, but ultimately as things
stand if the parser fails it calls strp_parser_err() with the error
which ends up in strp_abort_strp that should call
sk->sk_error_report(sk) but in kcm case sk is the csk and I guess
failing csk does not make a pending recv on the kcm sock to fail...

I'm not sure whether propagating the error to the upper socket is the
right thing to do, kcm is meant to be able to work with multiple
underlying sockets so I feel we must be cautious about that, but
strparser might be able to retry somehow.
This is what I said below:
> > [,,,]
> >  - 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.

Thanks,
-- 
Dominique

  reply	other threads:[~2019-02-15  3:31 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
2019-02-15  2:48               ` Tom Herbert
2019-02-15  3:31                 ` Dominique Martinet [this message]
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=20190215033102.GA3099@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).