On 2020/12/14 上午9:32, Willem de Bruijn wrote:
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.
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?
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
