[PATCH net-next] net: remove unsafe skb_insert()

[PATCH net-next] net: remove unsafe skb_insert()

[Eric Dumazet]

I do not see how one can effectively use skb_insert() without holding
some kind of lock. Otherwise other cpus could have changed the list
right before we have a chance of acquiring list->lock.

Only existing user is in drivers/infiniband/hw/nes/nes_mgt.c and this
one probably meant to use __skb_insert() since it appears nesqp->pau_list
is protected by nesqp->pau_lock. This looks like nesqp->pau_lock
could be removed, since nesqp->pau_list.lock could be used instead.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Faisal Latif <faisal.latif@intel.com>
Cc: Doug Ledford <dledford@redhat.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: linux-rdma <linux-rdma@vger.kernel.org>
---
 drivers/infiniband/hw/nes/nes_mgt.c |  4 ++--
 include/linux/skbuff.h              |  2 --
 net/core/skbuff.c                   | 22 ----------------------
 3 files changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/infiniband/hw/nes/nes_mgt.c b/drivers/infiniband/hw/nes/nes_mgt.c
index fc0c191014e908eea32d752f3499295ef143aa0a..abb54d30d35dd53fa983ee437506933eeba72746 100644
--- a/drivers/infiniband/hw/nes/nes_mgt.c
+++ b/drivers/infiniband/hw/nes/nes_mgt.c
@@ -551,14 +551,14 @@ static void queue_fpdus(struct sk_buff *skb, struct nes_vnic *nesvnic, struct ne
 
 	/* Queue skb by sequence number */
 	if (skb_queue_len(&nesqp->pau_list) == 0) {
-		skb_queue_head(&nesqp->pau_list, skb);
+		__skb_queue_head(&nesqp->pau_list, skb);
 	} else {
 		skb_queue_walk(&nesqp->pau_list, tmpskb) {
 			cb = (struct nes_rskb_cb *)&tmpskb->cb[0];
 			if (before(seqnum, cb->seqnum))
 				break;
 		}
-		skb_insert(tmpskb, skb, &nesqp->pau_list);
+		__skb_insert(tmpskb, skb, &nesqp->pau_list);
 	}
 	if (nesqp->pau_state == PAU_READY)
 		process_it = true;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f17a7452ac7bf47ef4bcf89840bba165cee6f50a..73902acf2b71c8800d81b744a936a7420f33b459 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1749,8 +1749,6 @@ static inline void skb_queue_head_init_class(struct sk_buff_head *list,
  *	The "__skb_xxxx()" functions are the non-atomic ones that
  *	can only be called with interrupts disabled.
  */
-void skb_insert(struct sk_buff *old, struct sk_buff *newsk,
-		struct sk_buff_head *list);
 static inline void __skb_insert(struct sk_buff *newsk,
 				struct sk_buff *prev, struct sk_buff *next,
 				struct sk_buff_head *list)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9a8a72cefe9b94d3821b9cc5ba5bba647ae51267..02cd7ae3d0fb26ef0a8b006390154fdefd0d292f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2990,28 +2990,6 @@ void skb_append(struct sk_buff *old, struct sk_buff *newsk, struct sk_buff_head
 }
 EXPORT_SYMBOL(skb_append);
 
-/**
- *	skb_insert	-	insert a buffer
- *	@old: buffer to insert before
- *	@newsk: buffer to insert
- *	@list: list to use
- *
- *	Place a packet before a given packet in a list. The list locks are
- * 	taken and this function is atomic with respect to other list locked
- *	calls.
- *
- *	A buffer cannot be placed on two lists at the same time.
- */
-void skb_insert(struct sk_buff *old, struct sk_buff *newsk, struct sk_buff_head *list)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&list->lock, flags);
-	__skb_insert(newsk, old->prev, old, list);
-	spin_unlock_irqrestore(&list->lock, flags);
-}
-EXPORT_SYMBOL(skb_insert);
-
 static inline void skb_split_inside_header(struct sk_buff *skb,
 					   struct sk_buff* skb1,
 					   const u32 len, const int pos)
-- 
2.20.0.rc0.387.gc7a69e6b6c-goog

Re: [PATCH net-next] net: remove unsafe skb_insert()

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Sun, 25 Nov 2018 08:26:23 -0800

> I do not see how one can effectively use skb_insert() without holding
> some kind of lock. Otherwise other cpus could have changed the list
> right before we have a chance of acquiring list->lock.
> 
> Only existing user is in drivers/infiniband/hw/nes/nes_mgt.c and this
> one probably meant to use __skb_insert() since it appears nesqp->pau_list
> is protected by nesqp->pau_lock. This looks like nesqp->pau_lock
> could be removed, since nesqp->pau_list.lock could be used instead.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Good find.

Indeed, any of the queue SKB manipulation functions that take two SKBs
as an argument are suspect in this manner.

Applied, thanks Eric.

Re: [PATCH net-next] net: remove unsafe skb_insert()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3909 bytes --]

Hi Eric,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Eric-Dumazet/net-remove-unsafe-skb_insert/20181126-061342
config: x86_64-randconfig-x009-201847 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/infiniband//hw/nes/nes_mgt.c: In function 'queue_fpdus':
>> drivers/infiniband//hw/nes/nes_mgt.c:561:29: error: passing argument 3 of '__skb_insert' from incompatible pointer type [-Werror=incompatible-pointer-types]
      __skb_insert(tmpskb, skb, &nesqp->pau_list);
                                ^
   In file included from drivers/infiniband//hw/nes/nes_mgt.c:34:0:
   include/linux/skbuff.h:1752:20: note: expected 'struct sk_buff *' but argument is of type 'struct sk_buff_head *'
    static inline void __skb_insert(struct sk_buff *newsk,
                       ^~~~~~~~~~~~
>> drivers/infiniband//hw/nes/nes_mgt.c:561:3: error: too few arguments to function '__skb_insert'
      __skb_insert(tmpskb, skb, &nesqp->pau_list);
      ^~~~~~~~~~~~
   In file included from drivers/infiniband//hw/nes/nes_mgt.c:34:0:
   include/linux/skbuff.h:1752:20: note: declared here
    static inline void __skb_insert(struct sk_buff *newsk,
                       ^~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/__skb_insert +561 drivers/infiniband//hw/nes/nes_mgt.c

   503	
   504	/**
   505	 * queue_fpdus - Handle fpdu's that hw passed up to sw
   506	 */
   507	static void queue_fpdus(struct sk_buff *skb, struct nes_vnic *nesvnic, struct nes_qp *nesqp)
   508	{
   509		struct sk_buff *tmpskb;
   510		struct nes_rskb_cb *cb;
   511		struct iphdr *iph;
   512		struct tcphdr *tcph;
   513		unsigned char *tcph_end;
   514		u32 rcv_nxt;
   515		u32 rcv_wnd;
   516		u32 seqnum;
   517		u32 len;
   518		bool process_it = false;
   519		unsigned long flags;
   520	
   521		/* Move data ptr to after tcp header */
   522		iph = (struct iphdr *)skb->data;
   523		tcph = (struct tcphdr *)(((char *)iph) + (4 * iph->ihl));
   524		seqnum = be32_to_cpu(tcph->seq);
   525		tcph_end = (((char *)tcph) + (4 * tcph->doff));
   526	
   527		len = be16_to_cpu(iph->tot_len);
   528		if (skb->len > len)
   529			skb_trim(skb, len);
   530		skb_pull(skb, tcph_end - skb->data);
   531	
   532		/* Initialize tracking values */
   533		cb = (struct nes_rskb_cb *)&skb->cb[0];
   534		cb->seqnum = seqnum;
   535	
   536		/* Make sure data is in the receive window */
   537		rcv_nxt = nesqp->pau_rcv_nxt;
   538		rcv_wnd = le32_to_cpu(nesqp->nesqp_context->rcv_wnd);
   539		if (!between(seqnum, rcv_nxt, (rcv_nxt + rcv_wnd))) {
   540			nes_mgt_free_skb(nesvnic->nesdev, skb, PCI_DMA_TODEVICE);
   541			nes_rem_ref_cm_node(nesqp->cm_node);
   542			return;
   543		}
   544	
   545		spin_lock_irqsave(&nesqp->pau_lock, flags);
   546	
   547		if (nesqp->pau_busy)
   548			nesqp->pau_pending = 1;
   549		else
   550			nesqp->pau_busy = 1;
   551	
   552		/* Queue skb by sequence number */
   553		if (skb_queue_len(&nesqp->pau_list) == 0) {
   554			__skb_queue_head(&nesqp->pau_list, skb);
   555		} else {
   556			skb_queue_walk(&nesqp->pau_list, tmpskb) {
   557				cb = (struct nes_rskb_cb *)&tmpskb->cb[0];
   558				if (before(seqnum, cb->seqnum))
   559					break;
   560			}
 > 561			__skb_insert(tmpskb, skb, &nesqp->pau_list);
   562		}
   563		if (nesqp->pau_state == PAU_READY)
   564			process_it = true;
   565		spin_unlock_irqrestore(&nesqp->pau_lock, flags);
   566	
   567		if (process_it)
   568			process_fpdus(nesvnic, nesqp);
   569	
   570		return;
   571	}
   572	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30370 bytes --]

Re: [PATCH net-next] net: remove unsafe skb_insert()

[Eric Dumazet]

On Sun, Nov 25, 2018 at 10:29 AM David Miller <davem@davemloft.net> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
> Date: Sun, 25 Nov 2018 08:26:23 -0800
>
> > I do not see how one can effectively use skb_insert() without holding
> > some kind of lock. Otherwise other cpus could have changed the list
> > right before we have a chance of acquiring list->lock.
> >
> > Only existing user is in drivers/infiniband/hw/nes/nes_mgt.c and this
> > one probably meant to use __skb_insert() since it appears nesqp->pau_list
> > is protected by nesqp->pau_lock. This looks like nesqp->pau_lock
> > could be removed, since nesqp->pau_list.lock could be used instead.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> Good find.
>
> Indeed, any of the queue SKB manipulation functions that take two SKBs
> as an argument are suspect in this manner.
>
> Applied, thanks Eric.

Oh well, this does not build.

Since you have not pushed your tree yet, maybe we can replace this
with a version that actually compiles.

Please let me know if a relative patch or a v2 is needed, thanks.

Re: [PATCH net-next] net: remove unsafe skb_insert()

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Sun, 25 Nov 2018 15:37:43 -0800

> On Sun, Nov 25, 2018 at 10:29 AM David Miller <davem@davemloft.net> wrote:
>>
>> From: Eric Dumazet <edumazet@google.com>
>> Date: Sun, 25 Nov 2018 08:26:23 -0800
>>
>> > I do not see how one can effectively use skb_insert() without holding
>> > some kind of lock. Otherwise other cpus could have changed the list
>> > right before we have a chance of acquiring list->lock.
>> >
>> > Only existing user is in drivers/infiniband/hw/nes/nes_mgt.c and this
>> > one probably meant to use __skb_insert() since it appears nesqp->pau_list
>> > is protected by nesqp->pau_lock. This looks like nesqp->pau_lock
>> > could be removed, since nesqp->pau_list.lock could be used instead.
>> >
>> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>>
>> Good find.
>>
>> Indeed, any of the queue SKB manipulation functions that take two SKBs
>> as an argument are suspect in this manner.
>>
>> Applied, thanks Eric.
> 
> Oh well, this does not build.
> 
> Since you have not pushed your tree yet, maybe we can replace this
> with a version that actually compiles.
> 
> Please let me know if a relative patch or a v2 is needed, thanks.

I fixed up the build in your original patch and am about to push that
out.

Re: [PATCH net-next] net: remove unsafe skb_insert()

[Eric Dumazet]

On 11/25/2018 07:52 PM, David Miller wrote:
> 
> I fixed up the build in your original patch and am about to push that
> out.

Thanks David, sorry for this, I should have compiled the damn thing :/