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  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?

Powered by blists - more mailing lists