RE: [PATCH net-next] net/ipv4/raw Optimise ipv4 raw sends when IP_HDRINCL set.

From: David Laight <David.Laight@ACULAB.COM>
To: 'David Miller' <davem@davemloft.net>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: RE: [PATCH net-next] net/ipv4/raw Optimise ipv4 raw sends when IP_HDRINCL set.
Date: Mon, 11 May 2020 21:28:18 +0000	[thread overview]
Message-ID: <7e8f6c9831244d2bb7c39f9aa4204e90@AcuMS.aculab.com> (raw)
In-Reply-To: <20200511.134938.651986318503897703.davem@davemloft.net>

From: David Miller
> Sent: 11 May 2020 21:50
> To: David Laight <David.Laight@ACULAB.COM>
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH net-next] net/ipv4/raw Optimise ipv4 raw sends when IP_HDRINCL set.
> 
> From: David Laight <David.Laight@ACULAB.COM>
> Date: Sun, 10 May 2020 16:00:41 +0000
> 
> > The final routing for ipv4 packets may be done with the IP address
> > from the message header not that from the address buffer.
> > If the addresses are different FLOWI_FLAG_KNOWN_NH must be set so
> > that a temporary 'struct rtable' entry is created to send the message.
> > However the allocate + free (under RCU) is relatively expensive
> > and can be avoided by a quick check shows the addresses match.
> >
> > Signed-off-by: David Laight <david.laight@aculab.com>
> 
> The user can change the daddr field in userspace between when you do
> this test and when the iphdr is copied into the sk_buff.
> 
> Also, you are obfuscating what you are doing in the way you have coded
> this check.  You extract 4 bytes from a magic offset (16), which is
> hard to understand.

Ok, that should at least be the structure offset.

> Just explicitly code out the fact that you are accessing the daddr
> field of an ip header.
> 
> But nonetheless you have to solve the "modified in userspace
> meanwhile" problem, as this is a bug we fix often in the kernel so we
> don't want to add new instances.

In this case the "modified in userspace meanwhile" just breaks the
application - it isn't any kind of security issue.

The problem is that you can't read the data into an skb until you
have the offset - which is got by looking up the destination address.
But you need the actual destination (from the packet data) to match
the address buffer if you don't want to create a temporary rtable entry.

I didn't find the commit that make rtable entries shared.
I though the same table was used for routes and arps  - but
it looks like they got separated at some point.

The code could put the address it read back into the skb, but that would
look even worse.

At the moment the performance is horrid when hundreds of the rtable
entries get deleted under rcu all together.
They are also added to a single locked linked list.

I'm running 500 RTP streams which each send one UDP message every 20ms.
It really needs to used the cached rtable entries.
The only sane way to send the data is through a raw socket and to get
the UDP checksum set you have to sort out the both IP addresses.
(A UPD socket would be given rx data - which needs to go elsewhere.)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)