On Fri, Feb 26, 2021 at 7:42 PM Jason A. Donenfeld <ja...@zx2c4.com> wrote:
>
> On Sat, Feb 27, 2021 at 12:29 AM Willem de Bruijn
> <willemdebruijn.ker...@gmail.com> wrote:
> >
> > On Fri, Feb 26, 2021 at 5:39 PM Jason A. Donenfeld <ja...@zx2c4.com> wrote:
> > >
> > > On Fri, Feb 26, 2021 at 10:25 PM Willem de Bruijn
> > > <willemdebruijn.ker...@gmail.com> wrote:
> > > >
> > > > On Thu, Feb 25, 2021 at 6:46 PM Jason A. Donenfeld <ja...@zx2c4.com>
> > > > wrote:
> > > > >
> > > > > There were a few remaining tunnel drivers that didn't receive the
> > > > > prior
> > > > > conversion to icmp{,v6}_ndo_send. Knowing now that this could lead to
> > > > > memory corrution (see ee576c47db60 ("net: icmp: pass zeroed opts from
> > > > > icmp{,v6}_ndo_send before sending") for details), there's even more
> > > > > imperative to have these all converted. So this commit goes through
> > > > > the
> > > > > remaining cases that I could find and does a boring translation to the
> > > > > ndo variety.
> > > > >
> > > > > Cc: Willem de Bruijn <will...@google.com>
> > > > > Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
> > > >
> > > > Using a stack variable over skb->cb[] is definitely the right fix for
> > > > all of these. Thanks Jason.
> > > >
> > > > Only part that I don't fully know is the conntrack conversion. That is
> > > > a behavioral change. What is the context behind that? I assume it's
> > > > fine. In that if needed, that is the case for all devices, nothing
> > > > specific about the couple that call icmp(v6)_ndo_send already.
> > >
> > > That's actually a sensible change anyway. icmp_send does something
> > > bogus if the packet has already passed through netfilter, which is why
> > > the ndo variant was adopted. So it's good and correct for these to
> > > change in that way.
> > >
> > > Jason
> >
> > Something bogus, how? Does this apply to all uses of conntrack?
> > Specifically NAT? Not trying to be obtuse, but I really find it hard
> > to evaluate that part.
>
> By the time packets hit ndo_start_xmit, the src address has changed,
> and icmp can't deliver to the actual source, and its rate limiting
> works against the wrong source. All of this was explained, justified,
> and discussed on the original icmp_ndo_start patchset, which included
> the function and converted drivers to use it. However, a few spots
> were missed, which this patchset cleans up. Here's the merge with
> details:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=803381f9f117493d6204d82445a530c834040fe6
Thanks for the link. I used git blame to a look at the patch that
introduced the code before asking for more context, but this state was
in the merge commit, so I missed it.
> The network devices that this patch here adjusts are no different than
> the four I originally fixed up in that series -- xfrmi, gtp, sunvnet,
> wireguard.
Agreed.
> > Please cc: the maintainers for patches that are meant to be merged, btw.
>
> Whoops. I'll do so and repost.
