Date:	Mon, 6 Sep 2010 13:35:05 +0300
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Changli Gao <xiaosuo@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: net: af_packet: skb_orphan should be avoided in TX path.

On Sun, Sep 05, 2010 at 07:43:55PM +0200, Eric Dumazet wrote:
> Le lundi 06 septembre 2010 à 01:18 +0800, Changli Gao a écrit :
> > af_packet uses tpacket_destruct_skb() to notify its user a frame is
> > sent out through NIC, and the memory for that frame is available for
> > the others. If the driver calls skb_orphan() before the frame is sent
> > out successfully, and the user may fill other data into the space for
> > this frame, this frame will be corrupted. It became more likely after
> > skb_try_orphan() was added into dev_hard_start_xmit().
> > 
> > Am I correct?
> > 
> 
> Yes good catch. We might add a :
> 
> SKBTX_NO_EARLY_ORPHAN = 1 << 4,
> 
> so that skb_orphan_try() do not early orphan this kind of skb
> 

I think there are bigger issues here.  As was pointed out, drivers might
orphan skbs before they transmit them.
And at least for tun, the reason is that we might hang on
to skbs indefinitely because userspace is not reading them.

So in that case, if you just prevent tun from orphaning skbs, the socket
will be prevented from sending any more packets out even if they are for
a completely unrelated destinations, right?
Further, module can't get unloaded and I think socket can not get
closed, so user can't kill the task which has the socket?

And thinking about this, I think I see
another issue related to the use of the destructor callback:

static void tpacket_destruct_skb(struct sk_buff *skb)
{
        struct packet_sock *po = pkt_sk(skb->sk);
        void *ph;

        BUG_ON(skb == NULL);

        if (likely(po->tx_ring.pg_vec)) {
                ph = skb_shinfo(skb)->destructor_arg;
                BUG_ON(__packet_get_status(po, ph) != TP_STATUS_SENDING);
                BUG_ON(atomic_read(&po->tx_ring.pending) == 0);
                atomic_dec(&po->tx_ring.pending);
                __packet_set_status(po, ph, TP_STATUS_AVAILABLE);
        }

        sock_wfree(skb);

<-----
at this point we still have to execute instructions
in this function to return from it. However
socket and thus module reference count
got already dropped to 0, so I think module could get unloaded
and these instructions could get overwritten.

}

I conclude that destructor callback should never point to a function residing
in a module, always to a function that is guaranteed to be builtin, this
function must be the one that drops the last module reference.

Comments?


> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index f900ffc..9c1a480 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -176,6 +176,9 @@ enum {
>  
>  	/* ensure the originating sk reference is available on driver level */
>  	SKBTX_DRV_NEEDS_SK_REF = 1 << 3,
> +
> +	/* dont early orphan this skb in skb_orphan_try() */
> +	SKBTX_NO_EARLY_ORPHAN = 1 << 4,
>  };
>  
>  /* This data is invariant across clones and lives at
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 3616f27..306795d 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1029,6 +1029,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>  		}
>  
>  		skb->destructor = tpacket_destruct_skb;
> +		skb_shinfo(skb)->tx_flags |= SKBTX_NO_EARLY_ORPHAN;
>  		__packet_set_status(po, ph, TP_STATUS_SENDING);
>  		atomic_inc(&po->tx_ring.pending);
>  
> 
> 
> --
> 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
--
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