Re: [PATCH net v2] net: icmp: pass zeroed opts from icmp{,v6}_ndo_send before sending

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Network Development <netdev@vger.kernel.org>, SinYu <liuxyon@gmail.com>
Subject: Re: [PATCH net v2] net: icmp: pass zeroed opts from icmp{,v6}_ndo_send before sending
Date: Thu, 18 Feb 2021 15:36:56 -0500	[thread overview]
Message-ID: <CAF=yD-K3wA5yTRSr7kas9xkKZwB2OcYOmqeOx4mpGoQfYCf7ZQ@mail.gmail.com> (raw)
In-Reply-To: <CAHmME9oyv+nWk2r3mcVrfdXW_aiex67nSvGiiqLmPOv=RHnhfQ@mail.gmail.com>

On Thu, Feb 18, 2021 at 3:25 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Thu, Feb 18, 2021 at 9:16 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Thu, Feb 18, 2021 at 12:58 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >
> > > On Thu, Feb 18, 2021 at 5:34 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > Thanks for respinning.
> > > >
> > > > Making ipv4 and ipv6 more aligned is a good goal, but more for
> > > > net-next than bug fixes that need to be backported to many stable
> > > > branches.
> > > >
> > > > Beyond that, I'm not sure this fixes additional cases vs the previous
> > > > patch? It uses new on-stack variables instead of skb->cb, which again
> > > > is probably good in general, but adds more change than is needed for
> > > > the stable fix.
> > >
> > > It doesn't appear to be problematic for applying to stable. I think
> > > this v2 is the "right way" to handle it. Zeroing out skb->cb is
> > > unexpected and weird anyway. What if the caller was expecting to use
> > > their skb->cb after calling icmp_ndo_send? Did they think it'd get
> > > wiped out like that? This v2 prevents that weird behavior from
> > > happening.
> > >
> > > > My comment on fixing all callers of  icmp{,v6}_send was wrong, in
> > > > hindsight. In most cases IPCB is set correctly before calling those,
> > > > so we cannot just zero inside those. If we can only address the case
> > > > for icmp{,v6}_ndo_send I think the previous patch introduced less
> > > > churn, so is preferable. Unless I'm missing something.
> > >
> > > As mentioned above it's weird and unexpected.
> > >
> > > > Reminder of two main comments: sufficient to zero sizeof(IPCB..) and
> > > > if respinning, please explicitly mention the path that leads to a
> > > > stack overflow, as it is not immediately obvious (even from reading
> > > > the fix code?).
> > >
> > > I don't intend to respin v1, as I think v2 is more correct, and I
> > > don't think only zeroing IPCB is a smart idea, as in the future that
> > > code is bound to break when somebody forgets to update it. This v2
> > > does away with the zeroing all together, though, so that the right
> > > bytes to be zeroed are properly enforced all the time by the type
> > > system.
> >
> > I'm afraid this latest version seems to have build issues, as per the
> > patchwork bot.
>
> Hmm I didn't get those bot emails. Either way, I'll do a bit of build
> testing with different config knobs now and send a v3. Thanks for
> letting me know.

Different bot :)

You might get emails from the other later. These can be found through
patchwork at

https://patchwork.kernel.org/project/netdevbpf/patch/20210218160745.2343501-1-Jason@zx2c4.com/