| Message ID | 20160601034300.GB31335@gondor.apana.org.au |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, May 31, 2016 at 8:43 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > When we postpone a broadcast packet we save the source port in > the skb if it is local. However, the source port can disappear > before we get a chance to process the packet. > Hmm, why could this happen? The upper device should be linked with the lower device, where a refcount is already held. Also, the work is cancelled in ->uninit().
On Tue, May 31, 2016 at 09:19:37PM -0700, Cong Wang wrote: > > Hmm, why could this happen? The upper device should be linked > with the lower device, where a refcount is already held. > Also, the work is cancelled in ->uninit(). Of course it can happen. We are talking about the source macvlan device that we just looked up using the Ethernet address. That device has nothing to do with the packet now so it may be deleted at any time. We do flush the work but only when the all macvlan devices on a port have been deleted. Perhaps you're confusing the source device with vlan->lowerdev which is confusingly the actual hardware device? Cheers,
On Tue, May 31, 2016 at 9:27 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Tue, May 31, 2016 at 09:19:37PM -0700, Cong Wang wrote: >> >> Hmm, why could this happen? The upper device should be linked >> with the lower device, where a refcount is already held. >> Also, the work is cancelled in ->uninit(). > > Of course it can happen. We are talking about the source macvlan > device that we just looked up using the Ethernet address. That > device has nothing to do with the packet now so it may be deleted > at any time. > > We do flush the work but only when the all macvlan devices on a > port have been deleted. Perhaps you're confusing the source > device with vlan->lowerdev which is confusingly the actual > hardware device? I thought all the on-flying packets are waited by synchronize_net() during the removal of any of these devices. But since you moved them to a workqueue, aka process context, so I think it won't work any more. Your patch makes sense to me now. :) Thanks!
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c index 2bcf1f3..78a00e3 100644 --- a/drivers/net/macvlan.c +++ b/drivers/net/macvlan.c @@ -305,11 +305,14 @@ static void macvlan_process_broadcast(struct work_struct *w) rcu_read_unlock(); + if (src) + dev_put(src->dev); kfree_skb(skb); } } static void macvlan_broadcast_enqueue(struct macvlan_port *port, + const struct macvlan_dev *src, struct sk_buff *skb) { struct sk_buff *nskb; @@ -319,8 +322,12 @@ static void macvlan_broadcast_enqueue(struct macvlan_port *port, if (!nskb) goto err; + MACVLAN_SKB_CB(nskb)->src = src; + spin_lock(&port->bc_queue.lock); if (skb_queue_len(&port->bc_queue) < MACVLAN_BC_QUEUE_LEN) { + if (src) + dev_hold(src->dev); __skb_queue_tail(&port->bc_queue, nskb); err = 0; } @@ -429,8 +436,7 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb) goto out; } - MACVLAN_SKB_CB(skb)->src = src; - macvlan_broadcast_enqueue(port, skb); + macvlan_broadcast_enqueue(port, src, skb); return RX_HANDLER_PASS; }
When we postpone a broadcast packet we save the source port in the skb if it is local. However, the source port can disappear before we get a chance to process the packet. This patch fixes this by holding a ref count on the netdev. It also delays the skb->cb modification until after we allocate the new skb as you should not modify shared skbs. Fixes: 412ca1550cbe ("macvlan: Move broadcasts into a work queue") Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>