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

[Jason Wang <jasowang@redhat.com>]

On 2020/12/14 上午9:32, Willem de Bruijn wrote:
> On Sat, Dec 12, 2020 at 7:18 PM Willem de Bruijn
> <willemdebruijn.kernel@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?