Re: net: af_packet: skb_orphan should be avoided in TX path.

[Eric Dumazet <eric.dumazet@gmail.com>]

Le lundi 06 septembre 2010 à 13:35 +0300, Michael S. Tsirkin a écrit :

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

It would be a surprise to use tx mmap (presumably to get high
performance), and a modular af_unix ;)

> 
> Comments?

The whole thing (packet / tx mmap) is broken, if you ask me.

skb_orphan() is not about protecting data, but doing per socket memory
accounting.

We have to skb_orphan() while data is still in use by skb, not only in
drivers but in core network stack. (loopback case for example, no need
to think about TUN being special ;) )

So I believe using mmap and tx on af_unix is racy in its current design.

We probably can remove some skb_orphan() calls (now its done in core
network, no real need to make it from some drivers), but not have a
complete solution to the problem Changli raised, without adding yet
another field into skb_shared_info...