netdev - Re: [PATCH] net: increase skb->users instead of skb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1292483902.2603.62.camel@edumazet-laptop>
Date:	Thu, 16 Dec 2010 08:18:22 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Changli Gao <xiaosuo@...il.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Tom Herbert <therbert@...gle.com>,
	Jiri Pirko <jpirko@...hat.com>,
	Fenghua Yu <fenghua.yu@...el.com>,
	Junchang Wang <junchangwang@...il.com>,
	Xinan Tang <xinan.tang@...el.com>, netdev@...r.kernel.org
Subject: Re: [PATCH] net: increase skb->users instead of skb_clone()

Le jeudi 16 décembre 2010 à 13:57 +0800, Changli Gao a écrit :
> In dev_queue_xmit_nit(), we have to clone skbs as we need to mangle skbs,
> however, we don't need to clone skbs for all the packet_types.
> 
> Except for the first packet_type, we increase skb->users instead of
> skb_clone().
> 
> Signed-off-by: Changli Gao <xiaosuo@...il.com>
> ---
>  net/core/dev.c |   30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index bf5ced5..888cb74 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1496,6 +1496,14 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL_GPL(dev_forward_skb);
>  
> +static inline int deliver_skb(struct sk_buff *skb,
> +			      struct packet_type *pt_prev,
> +			      struct net_device *orig_dev)
> +{
> +	atomic_inc(&skb->users);
> +	return pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
> +}
> +
>  /*
>   *	Support routine. Sends outgoing frames to any network
>   *	taps currently in use.
> @@ -1504,6 +1512,8 @@ EXPORT_SYMBOL_GPL(dev_forward_skb);
>  static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>  {
>  	struct packet_type *ptype;
> +	struct sk_buff *skb2 = NULL;
> +	struct packet_type *pt_prev = NULL;
>  
>  #ifdef CONFIG_NET_CLS_ACT
>  	if (!(skb->tstamp.tv64 && (G_TC_FROM(skb->tc_verd) & AT_INGRESS)))
> @@ -1520,7 +1530,13 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>  		if ((ptype->dev == dev || !ptype->dev) &&
>  		    (ptype->af_packet_priv == NULL ||
>  		     (struct sock *)ptype->af_packet_priv != skb->sk)) {
> -			struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);
> +			if (pt_prev) {
> +				deliver_skb(skb2, pt_prev, skb->dev);
> +				pt_prev = ptype;
> +				continue;
> +			}
> +
> +			skb2 = skb_clone(skb, GFP_ATOMIC);
>  			if (!skb2)
>  				break;
>  
> @@ -1542,9 +1558,11 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>  
>  			skb2->transport_header = skb2->network_header;
>  			skb2->pkt_type = PACKET_OUTGOING;
> -			ptype->func(skb2, skb->dev, ptype, skb->dev);
> +			pt_prev = ptype;
>  		}
>  	}
> +	if (pt_prev)
> +		pt_prev->func(skb2, skb->dev, pt_prev, skb->dev);
>  	rcu_read_unlock();
>  }
>  
> @@ -2788,14 +2806,6 @@ static void net_tx_action(struct softirq_action *h)
>  	}
>  }
>  
> -static inline int deliver_skb(struct sk_buff *skb,
> -			      struct packet_type *pt_prev,
> -			      struct net_device *orig_dev)
> -{
> -	atomic_inc(&skb->users);
> -	return pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
> -}
> -
>  #if (defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)) && \
>      (defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE))
>  /* This hook is defined here for ATM LANE */

You beat me, but I was thinking of a different way, adding a new
pt_prev->xmit_func(), handling all the details (no need for atomic ops
on skb users if packet is not delivered at all).

By the way, your patch is not 100% safe/OK, because af_packet rcv()
handler writes on skb (skb_pull() and all)



--
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