On Thu, Feb 18, 2021 at 9:16 PM Willem de Bruijn
<willemdebruijn.ker...@gmail.com> wrote:
>
> On Thu, Feb 18, 2021 at 12:58 PM Jason A. Donenfeld <ja...@zx2c4.com> wrote:
> >
> > On Thu, Feb 18, 2021 at 5:34 PM Willem de Bruijn
> > <willemdebruijn.ker...@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.
Jason
