Date:	Tue, 25 Mar 2014 11:05:14 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	xiyou.wangcong@...il.com, mpm@...enic.com, satyam.sharma@...il.com
Subject: Re: [PATCH 01/54] uml/net_kern: Call dev_consume_skb_any instead of dev_kfree_skb.

Eric Dumazet <eric.dumazet@...il.com> writes:

> On Mon, 2014-03-24 at 23:04 -0700, Eric W. Biederman wrote:
>> From: "Eric W. Biederman" <ebiederm@...ssion.com>
>> 
>> Replace dev_kfree_skb with dev_consume_skb_any in uml_net_start_xmit
>> as it can be called in hard irq and other contexts.
>> 
>> dev_consume_skb_any is used as uml_net_start_xmit typically
>> consumes (not drops) packets.
>
> Well, this is not exactly true. This driver certainly can drop packets.
>
> Here is an untested/not even compiled patch.

I said typically which does make it true :P.

I care because I trying to keep us from calling kfree_skb or consume_skb
aka dev_kfree_skb in hard irq context, as that can result in nasty
issues.

Since I am touching those places I am doing my best to pick the
correct consume or kfree variant that matches what the code does,
and there isn't always one.  But that is all at a best effort so I can
preserve the code logic.

These patches are deliberately very conservative so I can successfully
make and test them with simply code inspection.

I really don't think using enum skb_free_reason makes any sense
whatsoever.  Not in the implementation of dev_kfree_skb_any and
dev_kfree_skb_irq and certainly not in a driver.  What
net/core/drop_monitor.c wants is the address of the function where drops
occur (so we can track down and debug why the kernel is dropping
packets) and the existing implementation of dev_kfree_skb_any and
dev_kfree_skb_irq loose that information.  The use of enum
skb_free_reason is a big part of the reason why we loose that
information.  (We should be using a (void *) so that we can capture
__builtin_return_address(0) instead.  Your expanded use of enum
skb_free_reason below seems to encourage the bad implemenation and make
it even harder to fix dev_kfree_skb_any and dev_kfree_skb_irq.

In other locations with the same logic I justified the change as not
changing semantics when going from consume_skb (aka dev_kfree_skb) to
dev_consume_skb.  My apologies for not mentioning that in uml/net_kern.
I think not causing a regression in the kfree/consume distinction is
more important than getting the code exactly right.

If you are really interested in seeing us get the consume_skb vs
kfree_skb difference correct in drivers I recommend an audit of drivers
yourself.  A few weeks ago when I started looking at this there
was exactly one instance of dev_consume_skb_any or was it just
consume_skb in the entire driver tree.

So I really think getting the drop vs consume distinction perfect right
now is silly when it is hard to test and we have so much low hanging
fruit where the distinction was not even recognized.

So please ack my patches (especially this one) on the basis of execution
context correctness and drop/kfree distinction no regression or best
effort correctness.

Eric

> diff --git a/arch/um/drivers/net_kern.c b/arch/um/drivers/net_kern.c
> index 39f186252e02..8d1df7ed759e 100644
> --- a/arch/um/drivers/net_kern.c
> +++ b/arch/um/drivers/net_kern.c
> @@ -212,6 +212,7 @@ static int uml_net_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	struct uml_net_private *lp = netdev_priv(dev);
>  	unsigned long flags;
>  	int len;
> +	enum skb_free_reason reason = SKB_REASON_CONSUMED;
>  
>  	netif_stop_queue(dev);
>  
> @@ -228,19 +229,18 @@ static int uml_net_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  		/* this is normally done in the interrupt when tx finishes */
>  		netif_wake_queue(dev);
> -	}
> -	else if (len == 0) {
> -		netif_start_queue(dev);
> -		dev->stats.tx_dropped++;
> -	}
> -	else {
> +	} else {
> +		reason = SKB_REASON_DROPPED;
>  		netif_start_queue(dev);
> -		printk(KERN_ERR "uml_net_start_xmit: failed(%d)\n", len);
> +		if (len == 0)
> +			dev->stats.tx_dropped++;
> +		else
> +			pr_err("uml_net_start_xmit: failed(%d)\n", len);
>  	}
>  
>  	spin_unlock_irqrestore(&lp->lock, flags);
>  
> -	dev_kfree_skb(skb);
> +	__dev_kfree_skb_any(skb, reason);
>  
>  	return NETDEV_TX_OK;
>  }
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists