diff mbox series

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

Message ID 1607517703-18472-1-git-send-email-wangyunjian@huawei.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net,v2] tun: fix ubuf refcount incorrectly on error path | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/source_inline fail Was 0 now: 1
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 92 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

wangyunjian Dec. 9, 2020, 12:41 p.m. UTC
From: Yunjian Wang <wangyunjian@huawei.com>

After setting callback for ubuf_info of skb, the callback
(vhost_net_zerocopy_callback) will be called to decrease
the refcount when freeing skb. But when an exception occurs
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.

Fixes: 4477138fa0ae ("tun: properly test for IFF_UP")
Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver")

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
v2:
   Updated code, fix by delay copying ubuf_info
---
 drivers/net/tun.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

Comments

Willem de Bruijn Dec. 9, 2020, 2:43 p.m. UTC | #1
On Wed, Dec 9, 2020 at 8:03 AM wangyunjian <wangyunjian@huawei.com> wrote:
>
> From: Yunjian Wang <wangyunjian@huawei.com>
>
> After setting callback for ubuf_info of skb, the callback
> (vhost_net_zerocopy_callback) will be called to decrease
> the refcount when freeing skb. But when an exception occurs

With exception, you mean if tun_get_user returns an error that
propagates to the sendmsg call in vhost handle_tx, correct?

> 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?

Or alternatively clear the destructor in drop:

>
> Fixes: 4477138fa0ae ("tun: properly test for IFF_UP")
> Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver")
>
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
> v2:
>    Updated code, fix by delay copying ubuf_info
> ---
>  drivers/net/tun.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 2dc1988a8973..2ea822328e73 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1637,6 +1637,20 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>         return NULL;
>  }
>
> +/* copy ubuf_info for callback when skb has no error */
> +static inline void tun_copy_ubuf_info(struct sk_buff *skb, bool zerocopy, void *msg_control)
> +{
> +       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);
> +       }
> +}
> +
>  /* Get packet from user space buffer */
>  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>                             void *msg_control, struct iov_iter *from,
> @@ -1812,16 +1826,6 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>                 break;
>         }
>
> -       /* 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);
> -       }
> -
>         skb_reset_network_header(skb);
>         skb_probe_transport_header(skb);
>         skb_record_rx_queue(skb, tfile->queue_index);
> @@ -1830,6 +1834,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>                 struct bpf_prog *xdp_prog;
>                 int ret;
>
> +               tun_copy_ubuf_info(skb, zerocopy, msg_control);
>                 local_bh_disable();
>                 rcu_read_lock();
>                 xdp_prog = rcu_dereference(tun->xdp_prog);
> @@ -1881,6 +1886,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>                         return -ENOMEM;
>                 }
>
> +               tun_copy_ubuf_info(skb, zerocopy, msg_control);
>                 local_bh_disable();
>                 napi_gro_frags(&tfile->napi);
>                 local_bh_enable();
> @@ -1889,6 +1895,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>                 struct sk_buff_head *queue = &tfile->sk.sk_write_queue;
>                 int queue_len;
>
> +               tun_copy_ubuf_info(skb, zerocopy, msg_control);
>                 spin_lock_bh(&queue->lock);
>                 __skb_queue_tail(queue, skb);
>                 queue_len = skb_queue_len(queue);
> @@ -1899,8 +1906,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>
>                 local_bh_enable();
>         } else if (!IS_ENABLED(CONFIG_4KSTACKS)) {
> +               tun_copy_ubuf_info(skb, zerocopy, msg_control);
>                 tun_rx_batched(tun, tfile, skb, more);
>         } else {
> +               tun_copy_ubuf_info(skb, zerocopy, msg_control);
>                 netif_rx_ni(skb);
>         }
>         rcu_read_unlock();
> --
> 2.23.0
>
wangyunjian Dec. 12, 2020, 6:43 a.m. UTC | #2
> -----Original Message-----
> From: Willem de Bruijn [mailto:willemdebruijn.kernel@gmail.com]
> Sent: Wednesday, December 9, 2020 10:43 PM
> To: wangyunjian <wangyunjian@huawei.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>; Jason Wang
> <jasowang@redhat.com>; virtualization@lists.linux-foundation.org; Network
> Development <netdev@vger.kernel.org>; Lilijun (Jerry)
> <jerry.lilijun@huawei.com>; chenchanghu <chenchanghu@huawei.com>;
> xudingke <xudingke@huawei.com>
> Subject: Re: [PATCH net v2] tun: fix ubuf refcount incorrectly on error path
> 
> On Wed, Dec 9, 2020 at 8:03 AM wangyunjian <wangyunjian@huawei.com>
> wrote:
> >
> > From: Yunjian Wang <wangyunjian@huawei.com>
> >
> > After setting callback for ubuf_info of skb, the callback
> > (vhost_net_zerocopy_callback) will be called to decrease the refcount
> > when freeing skb. But when an exception occurs
> 
> With exception, you mean if tun_get_user returns an error that propagates to
> the sendmsg call in vhost handle_tx, correct?

Yes

> 
> > 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.

Thanks
> 
> >
> > Fixes: 4477138fa0ae ("tun: properly test for IFF_UP")
> > Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP
> > driver")
> >
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> > ---
> > v2:
> >    Updated code, fix by delay copying ubuf_info
> > ---
> >  drivers/net/tun.c | 29 +++++++++++++++++++----------
> >  1 file changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c index
> > 2dc1988a8973..2ea822328e73 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1637,6 +1637,20 @@ static struct sk_buff *tun_build_skb(struct
> tun_struct *tun,
> >         return NULL;
> >  }
> >
> > +/* copy ubuf_info for callback when skb has no error */ static inline
> > +void tun_copy_ubuf_info(struct sk_buff *skb, bool zerocopy, void
> > +*msg_control) {
> > +       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);
> > +       }
> > +}
> > +
> >  /* Get packet from user space buffer */  static ssize_t
> > tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> >                             void *msg_control, struct iov_iter *from,
> > @@ -1812,16 +1826,6 @@ static ssize_t tun_get_user(struct tun_struct
> *tun, struct tun_file *tfile,
> >                 break;
> >         }
> >
> > -       /* 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);
> > -       }
> > -
> >         skb_reset_network_header(skb);
> >         skb_probe_transport_header(skb);
> >         skb_record_rx_queue(skb, tfile->queue_index); @@ -1830,6
> > +1834,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct
> tun_file *tfile,
> >                 struct bpf_prog *xdp_prog;
> >                 int ret;
> >
> > +               tun_copy_ubuf_info(skb, zerocopy, msg_control);
> >                 local_bh_disable();
> >                 rcu_read_lock();
> >                 xdp_prog = rcu_dereference(tun->xdp_prog); @@
> -1881,6
> > +1886,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct
> tun_file *tfile,
> >                         return -ENOMEM;
> >                 }
> >
> > +               tun_copy_ubuf_info(skb, zerocopy, msg_control);
> >                 local_bh_disable();
> >                 napi_gro_frags(&tfile->napi);
> >                 local_bh_enable();
> > @@ -1889,6 +1895,7 @@ static ssize_t tun_get_user(struct tun_struct *tun,
> struct tun_file *tfile,
> >                 struct sk_buff_head *queue =
> &tfile->sk.sk_write_queue;
> >                 int queue_len;
> >
> > +               tun_copy_ubuf_info(skb, zerocopy, msg_control);
> >                 spin_lock_bh(&queue->lock);
> >                 __skb_queue_tail(queue, skb);
> >                 queue_len = skb_queue_len(queue); @@ -1899,8
> +1906,10
> > @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file
> > *tfile,
> >
> >                 local_bh_enable();
> >         } else if (!IS_ENABLED(CONFIG_4KSTACKS)) {
> > +               tun_copy_ubuf_info(skb, zerocopy, msg_control);
> >                 tun_rx_batched(tun, tfile, skb, more);
> >         } else {
> > +               tun_copy_ubuf_info(skb, zerocopy, msg_control);
> >                 netif_rx_ni(skb);
> >         }
> >         rcu_read_unlock();
> > --
> > 2.23.0
> >
Willem de Bruijn Dec. 13, 2020, 12:17 a.m. UTC | #3
> > > 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, 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 Dec. 14, 2020, 1:32 a.m. UTC | #4
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.

> 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 Dec. 14, 2020, 3:30 a.m. UTC | #5
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?
Willem de Bruijn Dec. 14, 2020, 3:54 a.m. UTC | #6
On Sun, Dec 13, 2020 at 10:30 PM Jason Wang <jasowang@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@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.
Willem de Bruijn Dec. 14, 2020, 3:56 a.m. UTC | #7
On Sun, Dec 13, 2020 at 10:54 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Sun, Dec 13, 2020 at 10:30 PM Jason Wang <jasowang@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@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.
Jason Wang Dec. 14, 2020, 4:06 a.m. UTC | #8
On 2020/12/14 上午11:56, Willem de Bruijn wrote:
> On Sun, Dec 13, 2020 at 10:54 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Sun, Dec 13, 2020 at 10:30 PM Jason Wang <jasowang@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@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().

Thanks
wangyunjian Dec. 14, 2020, 6:56 a.m. UTC | #9
> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Monday, December 14, 2020 12:07 PM
> To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: wangyunjian <wangyunjian@huawei.com>; Michael S. Tsirkin
> <mst@redhat.com>; virtualization@lists.linux-foundation.org; Network
> Development <netdev@vger.kernel.org>; Lilijun (Jerry)
> <jerry.lilijun@huawei.com>; chenchanghu <chenchanghu@huawei.com>;
> xudingke <xudingke@huawei.com>; huangbin (J)
> <brian.huangbin@huawei.com>; Willem de Bruijn <willemb@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.kernel@gmail.com> wrote:
> >> On Sun, Dec 13, 2020 at 10:30 PM Jason Wang <jasowang@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@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
diff mbox series

Patch

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 2dc1988a8973..2ea822328e73 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1637,6 +1637,20 @@  static struct sk_buff *tun_build_skb(struct tun_struct *tun,
 	return NULL;
 }
 
+/* copy ubuf_info for callback when skb has no error */
+static inline void tun_copy_ubuf_info(struct sk_buff *skb, bool zerocopy, void *msg_control)
+{
+	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);
+	}
+}
+
 /* Get packet from user space buffer */
 static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 			    void *msg_control, struct iov_iter *from,
@@ -1812,16 +1826,6 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		break;
 	}
 
-	/* 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);
-	}
-
 	skb_reset_network_header(skb);
 	skb_probe_transport_header(skb);
 	skb_record_rx_queue(skb, tfile->queue_index);
@@ -1830,6 +1834,7 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		struct bpf_prog *xdp_prog;
 		int ret;
 
+		tun_copy_ubuf_info(skb, zerocopy, msg_control);
 		local_bh_disable();
 		rcu_read_lock();
 		xdp_prog = rcu_dereference(tun->xdp_prog);
@@ -1881,6 +1886,7 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 			return -ENOMEM;
 		}
 
+		tun_copy_ubuf_info(skb, zerocopy, msg_control);
 		local_bh_disable();
 		napi_gro_frags(&tfile->napi);
 		local_bh_enable();
@@ -1889,6 +1895,7 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		struct sk_buff_head *queue = &tfile->sk.sk_write_queue;
 		int queue_len;
 
+		tun_copy_ubuf_info(skb, zerocopy, msg_control);
 		spin_lock_bh(&queue->lock);
 		__skb_queue_tail(queue, skb);
 		queue_len = skb_queue_len(queue);
@@ -1899,8 +1906,10 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 
 		local_bh_enable();
 	} else if (!IS_ENABLED(CONFIG_4KSTACKS)) {
+		tun_copy_ubuf_info(skb, zerocopy, msg_control);
 		tun_rx_batched(tun, tfile, skb, more);
 	} else {
+		tun_copy_ubuf_info(skb, zerocopy, msg_control);
 		netif_rx_ni(skb);
 	}
 	rcu_read_unlock();