Re: [PATCH net v2] tun: fix ubuf refcount incorrectly on error path

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Dec 12, 2020 at 7:18 PM Willem de Bruijn
<willemdebruijn.kernel@xxxxxxxxx> wrote:
>
> > > > afterwards, the error handling in vhost handle_tx() will try to
> > > > decrease the same refcount again. This is wrong and fix this by delay
> > > > copying ubuf_info until we're sure there's no errors.
> > >
> > > I think the right approach is to address this in the error paths, rather than
> > > complicate the normal datapath.
> > >
> > > Is it sufficient to suppress the call to vhost_net_ubuf_put in the handle_tx
> > > sendmsg error path, given that vhost_zerocopy_callback will be called on
> > > kfree_skb?
> >
> > We can not call kfree_skb() until the skb was created.
> >
> > >
> > > Or alternatively clear the destructor in drop:
> >
> > The uarg->callback() is called immediately after we decide do datacopy
> > even if caller want to do zerocopy. If another error occurs later, the vhost
> > handle_tx() will try to decrease it again.
>
> Oh right, I missed the else branch in this path:
>
>         /* copy skb_ubuf_info for callback when skb has no error */
>         if (zerocopy) {
>                 skb_shinfo(skb)->destructor_arg = msg_control;
>                 skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
>                 skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
>         } else if (msg_control) {
>                 struct ubuf_info *uarg = msg_control;
>                 uarg->callback(uarg, false);
>         }
>
> So if handle_tx_zerocopy calls tun_sendmsg with ubuf_info (and thus a
> reference to release), there are these five options:
>
> 1. tun_sendmsg succeeds, ubuf_info is associated with skb.
>      reference released from kfree_skb calling vhost_zerocopy_callback later
>
> 2. tun_sendmsg succeeds, ubuf_info is released immediately, as skb is
> not zerocopy.
>
> 3. tun_sendmsg fails before creating skb, handle_tx_zerocopy correctly
> cleans up on receiving error from tun_sendmsg.
>
> 4. tun_sendmsg fails after creating skb, but with copying: decremented
> at branch shown above + again in handle_tx_zerocopy
>
> 5. tun_sendmsg fails after creating skb, with zerocopy: decremented at
> kfree_skb in drop: + again in handle_tx_zerocopy
>
> Since handle_tx_zerocopy has no idea whether on error 3, 4 or 5
> occurred,

Actually, it does. If sendmsg returns an error, it can test whether
vq->heads[nvq->upend_idx].len != VHOST_DMA_IN_PROGRESS.

> either all decrement-on-error cases must be handled by
> handle_tx_zerocopy or none.
>
> Your patch chooses the latter. Makes sense.
>
> But can this still go wrong if the xdp path is taken, but no program
> exists or the program returns XDP_PASS. And then the packet hits an
> error path, such as ! IFF_UP?
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization



[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux