netdev - Re: [PATCH v1 1/1] veth: Do not drop packets larger then the mtu set on the receiving side

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 11 Jan 2019 14:06:30 +0100
From:   Fredrik Gustavsson <gustfred@...il.com>
To:     Roopa Prabhu <roopa@...ulusnetworks.com>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Toshiaki Makita <makita.toshiaki@....ntt.co.jp>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH v1 1/1] veth: Do not drop packets larger then the mtu set
 on the receiving side

Den tors 10 jan. 2019 kl 16:39 skrev Roopa Prabhu <roopa@...ulusnetworks.com>:
>
> On Thu, Jan 10, 2019 at 6:21 AM Andrew Lunn <andrew@...n.ch> wrote:
> >
> > On Thu, Jan 10, 2019 at 02:26:55PM +0100, Fredrik Gustavsson wrote:
> > > commit affede4a779420bd8510ab937251a3796d3228df
> > > Author: Fredrik Gustavsson <gustfred@...il.com>
> > > Date:   Tue Jan 8 11:21:39 2019 +0100
> > >
> > > veth: Do not drop packets larger then the mtu set on the receiving side
> > >
> > > Currently veth drops all packets larger then the mtu set on the receiving
> > > end of the pair. This is inconsistent with most hardware ethernet drivers
> > > that happily receives packets up the the ethernet MTU independent of the
> > > configured MTU.
> >
> > I agree with your argument, but i wonder if there could be a better
> > implementation.
> >
> > ____dev_forward_skb() is on the hot path, so your additional check is
> > going to slow down packet forwarding for everybody, not just veth.
> > is_skb_forwardable() also does some additional checks which you are
> > now skipping. Is that O.K?
> >
> > Since we are talking about a veth pair here, you have access to both
> > ends of the link. Maybe consider if the mtu is changed on one end, you
> > also change it on the other?
> >
> > Lets see what others think of that.
> >
>
> adding a IFF_VETH just for this seems like an overkill. especially
> when there are other ways to indicate a virtual device type like
> rtnetlink kind.
> Also, its best to keep such checks local to veth, maybe with something
> along the lines of what Andrew suggests above.

Andrew: Nice to hear that my arguments were good.
You are totally right about the test in is_skb_forwardable
The one that is missed now is:
if (!(dev->flags & IFF_UP))
So of course moving the check inside is_skb_forwadable instead would
solved that.
The example provide was to show how the behaviour could be provoked.
For the actual use case I can not change to MTU on one of the
interface.

Roopa:
Overkill or not seems like a slim patch this way. Of course
dev_forward_skb version just for veth would of course be possible to
do. Just thought that kind of change would not be accepted with
duplicated code. Or do you mean in some other way to do it?

About the point of slowing down (not sure how much we actaully are
talking about) is a really good one. Since this is not only for veth.
Is it then better to do if for just veth with a veth specific
dev_forward_skb?