> -----Original Message----- > From: Jason Wang [mailto:jasow...@redhat.com] > Sent: Monday, December 14, 2020 12:07 PM > To: Willem de Bruijn <willemdebruijn.ker...@gmail.com> > Cc: wangyunjian <wangyunj...@huawei.com>; Michael S. Tsirkin > <m...@redhat.com>; virtualizat...@lists.linux-foundation.org; Network > Development <netdev@vger.kernel.org>; Lilijun (Jerry) > <jerry.lili...@huawei.com>; chenchanghu <chenchan...@huawei.com>; > xudingke <xudin...@huawei.com>; huangbin (J) > <brian.huang...@huawei.com>; Willem de Bruijn <will...@google.com> > Subject: Re: [PATCH net v2] tun: fix ubuf refcount incorrectly on error path > > > On 2020/12/14 上午11:56, Willem de Bruijn wrote: > > On Sun, Dec 13, 2020 at 10:54 PM Willem de Bruijn > > <willemdebruijn.ker...@gmail.com> wrote: > >> On Sun, Dec 13, 2020 at 10:30 PM Jason Wang <jasow...@redhat.com> > wrote: > >>> > >>> On 2020/12/14 上午9:32, Willem de Bruijn wrote: > >>>> On Sat, Dec 12, 2020 at 7:18 PM Willem de Bruijn > >>>> <willemdebruijn.ker...@gmail.com> 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. > >>> > >>> Just to make sure I understand this. Any reason for it can't be > >>> VHOST_DMA_IN_PROGRESS here? > >> It can be, and it will be if tun_sendmsg returns EINVAL before > >> assigning the skb destructor. > > I meant returns an error, not necessarily only EINVAL. > > > >> Only if tun_sendmsg released the zerocopy state through > >> kfree_skb->vhost_zerocopy_callback will it have been updated to > >> VHOST_DMA_DONE_LEN. And only then must the caller not try to release > >> the state again. > > > > > I see. So I tend to fix this in vhost instead of tun to be consistent with the > current error handling in handle_tx_zerocopy().
Agree, thanks for the suggestion. I'll send v3 patch according to your comments. > > Thanks
