| Message ID | 20181125162623.127762-1-edumazet@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [net-next] net: remove unsafe skb_insert() | expand |
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.
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
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.
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.
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 :/
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)
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(-)