| 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 |
| 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 |
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 >
> -----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 > >
> > > 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?
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?
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?
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.
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.
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
> -----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 --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();