netdev - Re: [PATCH v3] net: af_packet: don't call tpacket_destruct

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: <1285244970.2864.46.camel@edumazet-laptop>
Date:	Thu, 23 Sep 2010 14:29:30 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Changli Gao <xiaosuo@...il.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Oliver Hartkopp <socketcan@...tkopp.net>,
	"Michael S. Tsirkin" <mst@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v3] net: af_packet: don't call tpacket_destruct_skb()
 until the skb is sent out

Le jeudi 23 septembre 2010 à 18:15 +0800, Changli Gao a écrit :
> Since skb->destructor() is used to account socket memory, and maybe called
> before the skb is sent out, a corrupt skb maybe sent out finally.
> 
> A new destructor is added into structure skb_shared_info(), and it won't
> be called until the last reference to the data of an skb is put. af_packet
> uses this destructor instead.
> 
> Signed-off-by: Changli Gao <xiaosuo@...il.com>
> ---
> v3: rename destructor to data_destructor, destructor_arg to data_destructor_arg,
>     fix splice the skbs generated by AF_PACKET socket to the pipe.

I dont understand this.

Could you describe how splice(from af_packet to pipe) is possible with
af_packet send path ?

Also, on such risky patch, could you please avoid inserting cleanups ?
I am referring to these bits :



@@ -884,9 +883,8 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
        to_write = tp_len;
 
        if (sock->type == SOCK_DGRAM) {
-               err = dev_hard_header(skb, dev, ntohs(proto), addr,
-                               NULL, tp_len);
-               if (unlikely(err < 0))
+               if (unlikely(dev_hard_header(skb, dev, ntohs(proto), addr,
+                                            NULL, tp_len) < 0))
                        return -EINVAL;
        } else if (dev->hard_header_len) {
                /* net device doesn't like empty head */
@@ -897,8 +895,7 @@ static int tpacket_fill_skb(struct packet_sock *po,
struct sk_buff *skb,
                }
 
                skb_push(skb, dev->hard_header_len);
-               err = skb_store_bits(skb, 0, data,
-                               dev->hard_header_len);
+               err = skb_store_bits(skb, 0, data, dev->hard_header_len);
                if (unlikely(err))
                        return err;
 
@@ -906,7 +903,6 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
                to_write -= dev->hard_header_len;
        }
 
-       err = -EFAULT;
        page = virt_to_page(data);
        offset = offset_in_page(data);
        len_max = PAGE_SIZE - offset;



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