Jason Wang
2020-Dec-14 03:30 UTC
[PATCH net v2] tun: fix ubuf refcount incorrectly on error path
On 2020/12/14 ??9:32, Willem de Bruijn wrote:> On Sat, Dec 12, 2020 at 7:18 PM Willem de Bruijn > <willemdebruijn.kernel at 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? Thanks> >> 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?
Willem de Bruijn
2020-Dec-14 03:54 UTC
[PATCH net v2] tun: fix ubuf refcount incorrectly on error path
On Sun, Dec 13, 2020 at 10:30 PM Jason Wang <jasowang at 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.kernel at 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. 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.