[PATCH net 0/9] rxrpc: Fix use of skb_cow_data()

[PATCH net 0/9] rxrpc: Fix use of skb_cow_data()

[David Howells]

Here's a series of patches that fixes the use of skb_cow_data() in rxrpc.
The problem is that skb_cow_data() indirectly requires that the maximum
usage count on an sk_buff be 1, and it may generate an assertion failure in
pskb_expand_head() if not.

This can occur because rxrpc_input_data() may be still holding a ref when
it has just attached the sk_buff to the rx ring and given that attachment
its own ref.  If recvmsg happens fast enough, skb_cow_data() can see the
ref still held by the softirq handler.

Further, a packet may contain multiple subpackets, each of which gets its
own attachment to the ring and its own ref - also making skb_cow_data() go
bang.

Fix this by:

 (1) The DATA packet is currently parsed for subpackets twice by the input
     routines.  Parse it just once instead and make notes in the sk_buff
     private data.

 (2) Use the notes from (1) when attaching the packet to the ring multiple
     times.  Once the packet is attached to the ring, recvmsg can see it
     and cow it and start modifying it, so the softirq handler is not
     permitted to look inside it from that point.

 (3) Stick a second reference count in the skb private data, in struct
     rxrpc_skb_priv, to count the refs held by the ring on the packet.  All
     these refs together hold one single ref on the sk_buff main ref
     counter.

 (4) Pass the ref from the input code to the ring rather than getting an
     extra ref.  rxrpc_input_data() uses a ref on the second refcount to
     prevent the packet from evaporating under it.

 (5) rxkad currently calls skb_cow_data() once for each subpacket it needs
     to decrypt.  It should only be calling this once, however, so move
     that into recvmsg and only do it when we first see a new packet.

There's also a patch to improve the rxrpc_skb tracepoint to make sure that
Tx-derived buffers are identified separately from Rx-derived buffers in the
trace.


The patches are tagged here:

	git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
	rxrpc-fixes-20190820

and can also be found on the following branch:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes

David
---
David Howells (9):
      rxrpc: Pass the input handler's data skb reference to the Rx ring
      rxrpc: Improve jumbo packet counting
      rxrpc: Use info in skbuff instead of reparsing a jumbo packet
      rxrpc: Add a private skb flag to indicate transmission-phase skbs
      rxrpc: Abstract out rxtx ring cleanup
      rxrpc: Use the tx-phase skb flag to simplify tracing
      rxrpc: Add a shadow refcount in the skb private data
      rxrpc: Use shadow refcount for packets in the RxTx ring
      rxrpc: Only call skb_cow_data() once per packet


 include/trace/events/rxrpc.h |   62 +++++----
 net/rxrpc/ar-internal.h      |   18 ++-
 net/rxrpc/call_event.c       |    8 +
 net/rxrpc/call_object.c      |   33 ++---
 net/rxrpc/conn_event.c       |    6 -
 net/rxrpc/input.c            |  285 ++++++++++++++++++++++--------------------
 net/rxrpc/local_event.c      |    4 -
 net/rxrpc/output.c           |    6 -
 net/rxrpc/peer_event.c       |   10 +
 net/rxrpc/protocol.h         |    9 +
 net/rxrpc/recvmsg.c          |   58 +++++----
 net/rxrpc/rxkad.c            |   32 +----
 net/rxrpc/sendmsg.c          |   14 +-
 net/rxrpc/skbuff.c           |   72 +++++++++--
 14 files changed, 348 insertions(+), 269 deletions(-)

Re: [PATCH net 0/9] rxrpc: Fix use of skb_cow_data()

[David Howells]

Sorry, I forgot to add a tested-by.  Will resend.

David

Re: [PATCH net 0/9] rxrpc: Fix use of skb_cow_data()

[David Miller]

From: David Howells <dhowells@redhat.com>
Date: Thu, 22 Aug 2019 13:22:33 +0100

> Here's a series of patches that fixes the use of skb_cow_data() in rxrpc.
> The problem is that skb_cow_data() indirectly requires that the maximum
> usage count on an sk_buff be 1, and it may generate an assertion failure in
> pskb_expand_head() if not.

It sounds like you are effectively doing a late unshare when you have to
do in-place encryption.

Why don't you just do an skb_unshare() at the beginning when you know that
you'll need to do that?

Re: [PATCH net 0/9] rxrpc: Fix use of skb_cow_data()

[David Howells]

David Miller <davem@davemloft.net> wrote:

> Why don't you just do an skb_unshare() at the beginning when you know that
> you'll need to do that?

I was trying to defer any copying to process context rather than doing it in
softirq context to spend less time in softirq context - plus that way I can
use GFP_NOIO (kafs) or GFP_KERNEL (direct AF_RXRPC socket) rather than
GFP_ATOMIC if the api supports it.

I don't remember now why I used skb_cow_data() rather than skb_unshare() - but
it was probably because the former leaves the sk_buff object itself intact,
whereas the latter replaces it.  I can switch to using skb_unshare() instead.

Question for you: how likely is a newly received buffer, through a UDP socket,
to be 'cloned'?

David

Re: [PATCH net 0/9] rxrpc: Fix use of skb_cow_data()

[David Miller]

From: David Howells <dhowells@redhat.com>
Date: Fri, 23 Aug 2019 09:52:28 +0100

> Question for you: how likely is a newly received buffer, through a UDP socket,
> to be 'cloned'?

Very unlikely, I'd say.

Re: [PATCH net 0/9] rxrpc: Fix use of skb_cow_data()

[David Miller]

I'm marking this series "deferred" while you investigate skb_unshare()
etc.

[PATCH net 0/9] rxrpc: Fix use of skb_cow_data()

[David Howells]

Here's a series of patches that fixes the use of skb_cow_data() in rxrpc.
The problem is that skb_cow_data() indirectly requires that the maximum
usage count on an sk_buff be 1, and it may generate an assertion failure in
pskb_expand_head() if not.

This can occur because rxrpc_input_data() may be still holding a ref when
it has just attached the sk_buff to the rx ring and given that attachment
its own ref.  If recvmsg happens fast enough, skb_cow_data() can see the
ref still held by the softirq handler.

Further, a packet may contain multiple subpackets, each of which gets its
own attachment to the ring and its own ref - also making skb_cow_data() go
bang.

Fix this by:

 (1) The DATA packet is currently parsed for subpackets twice by the input
     routines.  Parse it just once instead and make notes in the sk_buff
     private data.

 (2) Use the notes from (1) when attaching the packet to the ring multiple
     times.  Once the packet is attached to the ring, recvmsg can see it
     and cow it and start modifying it, so the softirq handler is not
     permitted to look inside it from that point.

 (3) Stick a second reference count in the skb private data, in struct
     rxrpc_skb_priv, to count the refs held by the ring on the packet.  All
     these refs together hold one single ref on the sk_buff main ref
     counter.

 (4) Pass the ref from the input code to the ring rather than getting an
     extra ref.  rxrpc_input_data() uses a ref on the second refcount to
     prevent the packet from evaporating under it.

 (5) rxkad currently calls skb_cow_data() once for each subpacket it needs
     to decrypt.  It should only be calling this once, however, so move
     that into recvmsg and only do it when we first see a new packet.

There's also a patch to improve the rxrpc_skb tracepoint to make sure that
Tx-derived buffers are identified separately from Rx-derived buffers in the
trace.

Tested-by: Marc Dionne <marc.dionne@auristor.com>

The patches are tagged here:

	git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
	rxrpc-fixes-20190820

and can also be found on the following branch:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes

David
---
David Howells (9):
      rxrpc: Pass the input handler's data skb reference to the Rx ring
      rxrpc: Improve jumbo packet counting
      rxrpc: Use info in skbuff instead of reparsing a jumbo packet
      rxrpc: Add a private skb flag to indicate transmission-phase skbs
      rxrpc: Abstract out rxtx ring cleanup
      rxrpc: Use the tx-phase skb flag to simplify tracing
      rxrpc: Add a shadow refcount in the skb private data
      rxrpc: Use shadow refcount for packets in the RxTx ring
      rxrpc: Only call skb_cow_data() once per packet


 include/trace/events/rxrpc.h |   62 +++++----
 net/rxrpc/ar-internal.h      |   18 ++-
 net/rxrpc/call_event.c       |    8 +
 net/rxrpc/call_object.c      |   33 ++---
 net/rxrpc/conn_event.c       |    6 -
 net/rxrpc/input.c            |  285 ++++++++++++++++++++++--------------------
 net/rxrpc/local_event.c      |    4 -
 net/rxrpc/output.c           |    6 -
 net/rxrpc/peer_event.c       |   10 +
 net/rxrpc/protocol.h         |    9 +
 net/rxrpc/recvmsg.c          |   58 +++++----
 net/rxrpc/rxkad.c            |   32 +----
 net/rxrpc/sendmsg.c          |   14 +-
 net/rxrpc/skbuff.c           |   72 +++++++++--
 14 files changed, 348 insertions(+), 269 deletions(-)