| Message ID | CACVXFVOc0XZ+eLHGiVwKuiUResRk8Cj9MS4EPMx7k57a0tEJhA@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, Apr 21, 2012 at 9:49 AM, Ming Lei <tom.leiming@gmail.com> wrote: > On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li <huajun.li.lee@gmail.com> wrote: >> On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming@gmail.com> wrote: >>> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@gmail.com> wrote: >>>> >>>> Above patch has already been integrated to mainline. However, maybe >>>> there still exists another potentail use-after-free issue, here is a >>>> case: >>>> After release the lock in unlink_urbs(), defer_bh() may move >>>> current skb from rxq/txq to dev->done queue, even cause the skb be >>>> released. Then in next loop cycle, it can't refer to expected skb, and >>>> may Oops again. >>> >>> Could you explain in a bit detail? Why can't the expected skb be refered >>> to in next loop? >> >> >> unlink_urbs() complete handler >> -------------------------------------- >> ------------------------------------------------- >> spin_unlock_irqrestore() >> rx_complete() >> derver_bh() >> >> __skb_unlink() >> >> __skb_queue_tail(&dev->done, skb) =======> skb is moved to >> dev->done, and can be freed by usbnet_bh() >> skb_queue_walk_safe() >> tmp = skb->next ===> refer to freed skb > > I see the problem, so looks skb_queue_walk_safe is not safe. > I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe > is needed and it may become unsafe if skb is freed during current loop. > > But removing the 2nd 'tmp = skb->next' doesn't help the problem, because > tmp still may become freed after releasing lock. > >> If its state is x_done/tx_done/rx_cleanup, that means the the skb will >> be released soon, right? If so, it should avoid calling >> usb_unlink_urb(). > > Even though you can avoid calling unlink for completed URBs, the skbs > still may be freed in unlink path because complete handler will be triggered > by unlink and the referenced skb may be freed before next loop, so your > patch can't fix the oops. > > As far as I can think of, we can hold lock of done queue to forbid skb free > during unlinking. The below patch may fix the problem, are you OK > with it? Sorry, the patch still doesn't work since done.lock can't be held before calling unlinking. One simple solution is just always holding q->lock during the whole loop, like before commit 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d (net/usbnet: avoid recursive locking in usbnet_stop()), and take the per cpu flag trick to avoid holding q->lock in complete handler called by unlink, see idea in below link: http://marc.info/?l=linux-usb&m=133491718707499&w=2 In theory, it should be simple, just take avoiding policy. > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index db99536..a9809d4 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct > sk_buff_head *q) > struct sk_buff *skb, *skbnext; > int count = 0; > > - spin_lock_irqsave (&q->lock, flags); > + spin_lock_irqsave(&dev->done.lock, flags); > + spin_lock(&q->lock); > skb_queue_walk_safe(q, skb, skbnext) { > struct skb_data *entry; > struct urb *urb; > @@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, struct > sk_buff_head *q) > * handler(include defer_bh). > */ > usb_get_urb(urb); > - spin_unlock_irqrestore(&q->lock, flags); > + spin_unlock(&q->lock); > // during some PM-driven resume scenarios, > // these (async) unlinks complete immediately > retval = usb_unlink_urb (urb); > @@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev, > struct sk_buff_head *q) > else > count++; > usb_put_urb(urb); > - spin_lock_irqsave(&q->lock, flags); > + spin_lock(&q->lock); > } > - spin_unlock_irqrestore (&q->lock, flags); > + spin_unlock(&q->lock); > + spin_unlock_irqrestore(&dev->done.lock, flags); > return count; > } > > > > Thanks, > -- > Ming Lei
On Sat, Apr 21, 2012 at 9:49 AM, Ming Lei <tom.leiming@gmail.com> wrote: > On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li <huajun.li.lee@gmail.com> wrote: >> On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming@gmail.com> wrote: >>> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@gmail.com> wrote: >>>> >>>> Above patch has already been integrated to mainline. However, maybe >>>> there still exists another potentail use-after-free issue, here is a >>>> case: >>>> After release the lock in unlink_urbs(), defer_bh() may move >>>> current skb from rxq/txq to dev->done queue, even cause the skb be >>>> released. Then in next loop cycle, it can't refer to expected skb, and >>>> may Oops again. >>> >>> Could you explain in a bit detail? Why can't the expected skb be refered >>> to in next loop? >> >> >> unlink_urbs() complete handler >> -------------------------------------- >> ------------------------------------------------- >> spin_unlock_irqrestore() >> rx_complete() >> derver_bh() >> >> __skb_unlink() >> >> __skb_queue_tail(&dev->done, skb) =======> skb is moved to >> dev->done, and can be freed by usbnet_bh() >> skb_queue_walk_safe() >> tmp = skb->next ===> refer to freed skb > > I see the problem, so looks skb_queue_walk_safe is not safe. > I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe > is needed and it may become unsafe if skb is freed during current loop. > > But removing the 2nd 'tmp = skb->next' doesn't help the problem, because > tmp still may become freed after releasing lock. > >> If its state is x_done/tx_done/rx_cleanup, that means the the skb will >> be released soon, right? If so, it should avoid calling >> usb_unlink_urb(). > > Even though you can avoid calling unlink for completed URBs, the skbs > still may be freed in unlink path because complete handler will be triggered > by unlink and the referenced skb may be freed before next loop, so your > patch can't fix the oops. > Hi Ming, That's why my patch uses skb_queue_walk() to traverse the queue, it guarantees the skb available in each loop. Is this what you expected? The main idea of my patch(it is based on current mainline: 3.4.0-rc3) is: 1. If the skb in txq/rxq, then it must be available, unlink_urbs() can refer to it safely while it holds q->lock; 2. If the skb in txq/rxq and its state is rx_done/tx_done/rx_cleanup, that means the skb's URB is complete, then don't need to unlink it again; 3. Before releasing q->lock in unlink_urbs(), it will increase the URB's refercount, so even the related skb is freed in future, the URB is still available. Thanks, --Huajun > As far as I can think of, we can hold lock of done queue to forbid skb free > during unlinking. The below patch may fix the problem, are you OK > with it? Just skip trying this per your following email's comments. > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index db99536..a9809d4 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct > sk_buff_head *q) > struct sk_buff *skb, *skbnext; > int count = 0; > > - spin_lock_irqsave (&q->lock, flags); > + spin_lock_irqsave(&dev->done.lock, flags); > + spin_lock(&q->lock); > skb_queue_walk_safe(q, skb, skbnext) { > struct skb_data *entry; > struct urb *urb; > @@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, struct > sk_buff_head *q) > * handler(include defer_bh). > */ > usb_get_urb(urb); > - spin_unlock_irqrestore(&q->lock, flags); > + spin_unlock(&q->lock); > // during some PM-driven resume scenarios, > // these (async) unlinks complete immediately > retval = usb_unlink_urb (urb); > @@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev, > struct sk_buff_head *q) > else > count++; > usb_put_urb(urb); > - spin_lock_irqsave(&q->lock, flags); > + spin_lock(&q->lock); > } > - spin_unlock_irqrestore (&q->lock, flags); > + spin_unlock(&q->lock); > + spin_unlock_irqrestore(&dev->done.lock, flags); > return count; > } > > > > Thanks, > -- > Ming Lei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Huajun, On Sat, Apr 21, 2012 at 2:39 PM, Huajun Li <huajun.li.lee@gmail.com> wrote: > Hi Ming, > That's why my patch uses skb_queue_walk() to traverse the queue, > it guarantees the skb available in each loop. Is this what you > expected? > > The main idea of my patch(it is based on current mainline: 3.4.0-rc3) is: > 1. If the skb in txq/rxq, then it must be available, > unlink_urbs() can refer to it safely while it holds q->lock; > 2. If the skb in txq/rxq and its state is > rx_done/tx_done/rx_cleanup, that means the skb's URB is complete, then > don't need to unlink it again; As I said before, at least current code is not mistaken, since unlink can handle the case correctly. > 3. Before releasing q->lock in unlink_urbs(), it will increase > the URB's refercount, so even the related skb is freed in future, the > URB is still available. No, increasing URB's reference count does not prevent the referenced skb from being freed, see usbnet_bh(), so 'tmp = skb->next' in skb_queue_walk_safe still may reference a freed pointer. >> As far as I can think of, we can hold lock of done queue to forbid skb free >> during unlinking. The below patch may fix the problem, are you OK >> with it? > > Just skip trying this per your following email's comments. I will prepare a new patch later, if you'd like to try it. Thanks
On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei <tom.leiming@gmail.com> wrote: > Hi Huajun, > > On Sat, Apr 21, 2012 at 2:39 PM, Huajun Li <huajun.li.lee@gmail.com> wrote: > >> Hi Ming, >> That's why my patch uses skb_queue_walk() to traverse the queue, >> it guarantees the skb available in each loop. Is this what you >> expected? >> >> The main idea of my patch(it is based on current mainline: 3.4.0-rc3) is: >> 1. If the skb in txq/rxq, then it must be available, >> unlink_urbs() can refer to it safely while it holds q->lock; >> 2. If the skb in txq/rxq and its state is >> rx_done/tx_done/rx_cleanup, that means the skb's URB is complete, then >> don't need to unlink it again; > > As I said before, at least current code is not mistaken, since unlink > can handle the case correctly. > >> 3. Before releasing q->lock in unlink_urbs(), it will increase >> the URB's refercount, so even the related skb is freed in future, the >> URB is still available. > > No, increasing URB's reference count does not prevent the referenced skb > from being freed, see usbnet_bh(), so 'tmp = skb->next' in skb_queue_walk_safe > still may reference a freed pointer. > Did we on the same page, could you please review my patch again? My draft patch was based on current mainline( 3.4.0-rc3) which had already integrated your previous patch. And in my patch, it replaced skb_queue_walk_safe() with skb_queue_walk(), so you will not see 'tmp = skb->next' any more. >>> As far as I can think of, we can hold lock of done queue to forbid skb free >>> during unlinking. The below patch may fix the problem, are you OK >>> with it? >> >> Just skip trying this per your following email's comments. > > I will prepare a new patch later, if you'd like to try it. > > > Thanks > -- > Ming Lei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Huajun, On Sat, Apr 21, 2012 at 3:50 PM, Huajun Li <huajun.li.lee@gmail.com> wrote: > Did we on the same page, could you please review my patch again? > > My draft patch was based on current mainline( 3.4.0-rc3) which had > already integrated your previous patch. And in my patch, it replaced > skb_queue_walk_safe() with skb_queue_walk(), so you will not see 'tmp > = skb->next' any more. Replace skb_queue_walk_safe with skb_queue_walk doesn't improve the problem, since 'skb = skb->next' in skb_queue_walk still may trigger the oops, does it? Thanks,
On Sat, Apr 21, 2012 at 3:56 PM, Ming Lei <tom.leiming@gmail.com> wrote: > Hi Huajun, > > On Sat, Apr 21, 2012 at 3:50 PM, Huajun Li <huajun.li.lee@gmail.com> wrote: > >> Did we on the same page, could you please review my patch again? >> >> My draft patch was based on current mainline( 3.4.0-rc3) which had >> already integrated your previous patch. And in my patch, it replaced >> skb_queue_walk_safe() with skb_queue_walk(), so you will not see 'tmp >> = skb->next' any more. > > Replace skb_queue_walk_safe with skb_queue_walk doesn't improve > the problem, since 'skb = skb->next' in skb_queue_walk still may trigger > the oops, does it? > No. In each loop, my patch traverse the queue from its head, and it always holds q->lock when it need refer "skb->next", this can make sure the right skb is not moved out of rxq/txq. Can this fix what you concern? If so, IMO, there is no need to revert your previous patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Ming Lei <tom.leiming@gmail.com> Date: Sat, 21 Apr 2012 09:49:51 +0800 > I see the problem, so looks skb_queue_walk_safe is not safe. > I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe > is needed and it may become unsafe if skb is freed during current loop. I can't see what the problem is, skb_queue_walk_safe() is perfect and does exactly what it advertises to do. If 'skb' is unlinked inside of an skb_queue_walk_safe() loop, that's fine, because we won't touch 'skb' in the loop iteration tail code. Instead, before the loop contents, we pre-fetch skb->next into 'tmp' and then at the end we move 'skb' forward by simply assigning 'tmp'. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Apr 22, 2012 at 3:23 AM, David Miller <davem@davemloft.net> wrote: > From: Ming Lei <tom.leiming@gmail.com> > Date: Sat, 21 Apr 2012 09:49:51 +0800 > >> I see the problem, so looks skb_queue_walk_safe is not safe. >> I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe >> is needed and it may become unsafe if skb is freed during current loop. > > I can't see what the problem is, skb_queue_walk_safe() is perfect > and does exactly what it advertises to do. > > If 'skb' is unlinked inside of an skb_queue_walk_safe() loop, that's > fine, because we won't touch 'skb' in the loop iteration tail code. > > Instead, before the loop contents, we pre-fetch skb->next into 'tmp' > and then at the end we move 'skb' forward by simply assigning 'tmp'. In this case, the problem is, 'tmp = skb->next' can be moved out of rxq/txq, and even be freed. Then in next loop cycle, 'skb = tmp' will refer to a freed skb. You know, in current code stack, unlink_urbs() releases q->lock in each loop, this gives chance to urb complete handler to call defer_bh() and cause the problem. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Huajun Li <huajun.li.lee@gmail.com> Date: Sun, 22 Apr 2012 09:45:55 +0800 > On Sun, Apr 22, 2012 at 3:23 AM, David Miller <davem@davemloft.net> wrote: >> From: Ming Lei <tom.leiming@gmail.com> >> Date: Sat, 21 Apr 2012 09:49:51 +0800 >> >>> I see the problem, so looks skb_queue_walk_safe is not safe. >>> I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe >>> is needed and it may become unsafe if skb is freed during current loop. >> >> I can't see what the problem is, skb_queue_walk_safe() is perfect >> and does exactly what it advertises to do. >> >> If 'skb' is unlinked inside of an skb_queue_walk_safe() loop, that's >> fine, because we won't touch 'skb' in the loop iteration tail code. >> >> Instead, before the loop contents, we pre-fetch skb->next into 'tmp' >> and then at the end we move 'skb' forward by simply assigning 'tmp'. > > In this case, the problem is, 'tmp = skb->next' can be moved out of > rxq/txq, and even be freed. Then in next loop cycle, 'skb = tmp' will > refer to a freed skb. You know, in current code stack, unlink_urbs() > releases q->lock in each loop, this gives chance to urb complete > handler to call defer_bh() and cause the problem. Right, just like interfaces such as list_for_each_entry_safe(), this macro isn't designed to handle cases where you unlink more than one entry in the list. Specifically, it's designed only to handle the case when you unlink the entry being processed in the current loop iteration. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index db99536..a9809d4 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) struct sk_buff *skb, *skbnext; int count = 0; - spin_lock_irqsave (&q->lock, flags); + spin_lock_irqsave(&dev->done.lock, flags); + spin_lock(&q->lock); skb_queue_walk_safe(q, skb, skbnext) { struct skb_data *entry;