netdev - Re: [PATCH 1/2 v2] af-packet: Use existing netdev reference for bound sockets.

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]

Date:	Fri, 27 May 2011 05:42:25 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	greearb@...delatech.com
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH 1/2 v2] af-packet:  Use existing netdev reference for
 bound sockets.

Le jeudi 26 mai 2011 à 16:55 -0700, greearb@...delatech.com a écrit :
> From: Ben Greear <greearb@...delatech.com>
> 
> This saves a network device lookup on each packet transmitted,
> for sockets that are bound to a network device.
> 
> Signed-off-by: Ben Greear <greearb@...delatech.com>
> ---
> 
> v2:  Remove un-used ifindex, lookup dev in else branch.
> 
> :100644 100644 4005b24... f9b807d... M	net/packet/af_packet.c
>  net/packet/af_packet.c |   27 +++++++++++++++------------
>  1 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 4005b24..f9b807d 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -989,7 +989,8 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>  	struct sk_buff *skb;
>  	struct net_device *dev;
>  	__be16 proto;
> -	int ifindex, err, reserve = 0;
> +	bool need_rls_dev = false;
> +	int err, reserve = 0;
>  	void *ph;
>  	struct sockaddr_ll *saddr = (struct sockaddr_ll *)msg->msg_name;
>  	int tp_len, size_max;
> @@ -1001,7 +1002,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>  
>  	err = -EBUSY;
>  	if (saddr == NULL) {
> -		ifindex	= po->ifindex;
> +		dev = po->prot_hook.dev;
>  		proto	= po->num;
>  		addr	= NULL;
>  	} else {
> @@ -1012,12 +1013,12 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>  					+ offsetof(struct sockaddr_ll,
>  						sll_addr)))
>  			goto out;
> -		ifindex	= saddr->sll_ifindex;
>  		proto	= saddr->sll_protocol;
>  		addr	= saddr->sll_addr;
> +		dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
> +		need_rls_dev = true;
>  	}
>  
> -	dev = dev_get_by_index(sock_net(&po->sk), ifindex);
>  	err = -ENXIO;
>  	if (unlikely(dev == NULL))
>  		goto out;
> @@ -1103,7 +1104,8 @@ out_status:
>  	__packet_set_status(po, ph, status);
>  	kfree_skb(skb);
>  out_put:
> -	dev_put(dev);
> +	if (need_rls_dev)
> +		dev_put(dev);
>  out:
>  	mutex_unlock(&po->pg_vec_lock);
>  	return err;
> @@ -1141,8 +1143,9 @@ static int packet_snd(struct socket *sock,
>  	struct sk_buff *skb;
>  	struct net_device *dev;
>  	__be16 proto;
> +	bool need_rls_dev = false;
>  	unsigned char *addr;
> -	int ifindex, err, reserve = 0;
> +	int err, reserve = 0;
>  	struct virtio_net_hdr vnet_hdr = { 0 };
>  	int offset = 0;
>  	int vnet_hdr_len;
> @@ -1160,7 +1163,7 @@ static int packet_snd(struct socket *sock,
>  	 */
>  
>  	if (saddr == NULL) {
> -		ifindex	= po->ifindex;
> +		dev = po->prot_hook.dev;
>  		proto	= po->num;
>  		addr	= NULL;
>  	} else {
> @@ -1169,13 +1172,12 @@ static int packet_snd(struct socket *sock,
>  			goto out;
>  		if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
>  			goto out;
> -		ifindex	= saddr->sll_ifindex;
>  		proto	= saddr->sll_protocol;
>  		addr	= saddr->sll_addr;
> +		dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
> +		need_rls_dev = true;
>  	}
>  
> -
> -	dev = dev_get_by_index(sock_net(sk), ifindex);
>  	err = -ENXIO;
>  	if (dev == NULL)
>  		goto out_unlock;
> @@ -1315,14 +1317,15 @@ static int packet_snd(struct socket *sock,
>  	if (err > 0 && (err = net_xmit_errno(err)) != 0)
>  		goto out_unlock;
>  
> -	dev_put(dev);
> +	if (need_rls_dev)
> +		dev_put(dev);
>  
>  	return len;
>  
>  out_free:
>  	kfree_skb(skb);
>  out_unlock:
> -	if (dev)
> +	if (dev && need_rls_dev)
>  		dev_put(dev);
>  out:
>  	return err;

Hmmm, I wonder why you want this Ben.

IMHO this is buggy, because we can sleep in this function.

We must take a ref on device (its really cheap these days, now we have a
percpu device refcnt)



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