netdev - Re: [PATCH net 3/5] sunvnet: use icmp_ndo

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHmME9qSQaWJi_npqX7YQFhCsuikvKhGe62o-ztEaD5h-zvd1A@mail.gmail.com>
Date:   Mon, 10 Feb 2020 14:34:24 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     David Miller <davem@...emloft.net>
Cc:     netfilter-devel@...r.kernel.org, Netdev <netdev@...r.kernel.org>,
        shannon.nelson@...cle.com
Subject: Re: [PATCH net 3/5] sunvnet: use icmp_ndo_send helper

On Mon, Feb 10, 2020 at 12:36 PM David Miller <davem@...emloft.net> wrote:
>
> From: "Jason A. Donenfeld" <Jason@...c4.com>
> 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@...c4.com>
>
> 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