Willem de Bruijn
2020-Dec-14 01:32 UTC
[PATCH net v2] tun: fix ubuf refcount incorrectly on error path
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.> 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?
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?