On Mon, Feb 10, 2020 at 12:36 PM David Miller <davem@xxxxxxxxxxxxx> wrote:
>
> From: "Jason A. Donenfeld" <Jason@xxxxxxxxx>
> Date: Sun, 9 Feb 2020 15:31:41 +0100
>
> > Because sunvnet is calling icmp from network device context, it should use
> > the ndo helper so that the rate limiting applies correctly.
> >
> > Signed-off-by: Jason A. Donenfeld <Jason@xxxxxxxxx>
>
> Two things, first you should resubmit this patch series with a proper
> header [PATCH 0/N ... ] posting.
Ack, will do for v2.
>
> Second:
>
> > @@ -1363,14 +1363,14 @@ sunvnet_start_xmit_common(struct sk_buff *skb, struct net_device *dev,
> > rt = ip_route_output_key(dev_net(dev), &fl4);
> > if (!IS_ERR(rt)) {
> > skb_dst_set(skb, &rt->dst);
> > - icmp_send(skb, ICMP_DEST_UNREACH,
> > - ICMP_FRAG_NEEDED,
> > - htonl(localmtu));
> > + icmp_ndo_send(skb, ICMP_DEST_UNREACH,
> > + ICMP_FRAG_NEEDED,
> > + htonl(localmtu));
> > }
> > }
>
> Well, obviously if the saddr could be wrong here then this invalidates
> the route lookup done in the lines above your changes.
>
> It looks like this code is just making sure the ICMP path is routable
> which is kinda bogus because that is the icmp code's job. So very
> likely the right thing to do is to remove all of that route lookup
> and check code entirely. And that matches what all the other instances
> of driver icmp calls in your patces do.
Good point. I'll simplify that by just getting rid of the superfluous
route lookup and make sure that the other ones are okay too.
Thanks,
Jason
