Re: [PATCH] net: always use icmp{,v6}_ndo_send from ndo_start_xmit

On Fri, Feb 26, 2021 at 7:42 PM Jason A. Donenfeld [off-list ref] wrote:

On Sat, Feb 27, 2021 at 12:29 AM Willem de Bruijn
[off-list ref] wrote:

quoted

On Fri, Feb 26, 2021 at 5:39 PM Jason A. Donenfeld [off-list ref] wrote:

quoted

On Fri, Feb 26, 2021 at 10:25 PM Willem de Bruijn
[off-list ref] wrote:

quoted

On Thu, Feb 25, 2021 at 6:46 PM Jason A. Donenfeld [off-list ref] wrote:

quoted

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 <willemb@google.com>
Signed-off-by: Jason A. Donenfeld <Jason@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.

quoted

Please cc: the maintainers for patches that are meant to be merged, btw.

Whoops. I'll do so and repost.