Re: [PATCH v2 3/3] pppoatm: protect against freeing of vcc

On Thu, Nov 29, 2012 at 03:47:57PM +0000, David Woodhouse wrote:

On Thu, 2012-11-29 at 16:09 +0100, Krzysztof Mazur wrote:

quoted

I don't like two thinks about this patch:

        - if allos_skb(sizeof(*header), GFP_ATOMIC) at beginning of
          pclose() fails we will crash

        - if card wakes up after this timeout we will probably crash too

That's why proposed different approach, but it has other problems.

How about this variant on what you suggested. Yes, we can definitely
remove everything that's in the queue... as long as we use
skb_queue_walk_safe() instead of skb_queue_walk().

We can use GFP_KERNEL instead of GFP_ATOMIC, which at least reduces the
likelihood of failing to close the vcc.

We end up waiting *only* if there is a packet which is *currently* being
DMA'd to the card. And if the card doesn't take that within 5 seconds,
it almost certainly never will. So I can live with that.

Yeah, that shouldn't happen.

+				if (!test_bit(ATM_VF_READY, &vcc->flags))
+					wake_up(&card->param_wq);
+			} else

according to CodingStyle:

+ } else {

 				dev_kfree_skb_irq(oldskb);
-			}

+ }

Krzysiek