| Message ID | 1606982459-41752-1-git-send-email-wangyunjian@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] 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-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| 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 81 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 2020/12/3 下午4:00, wangyunjian 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 > afterwards, the error handling in vhost handle_tx() will > try to decrease the same refcount again. This is wrong and > fix this by clearing ubuf_info when meeting 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> > --- > drivers/net/tun.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 2dc1988a8973..3614bb1b6d35 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1861,6 +1861,12 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > if (unlikely(!(tun->dev->flags & IFF_UP))) { > err = -EIO; > rcu_read_unlock(); > + if (zerocopy) { > + skb_shinfo(skb)->destructor_arg = NULL; > + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY; > + skb_shinfo(skb)->tx_flags &= ~SKBTX_SHARED_FRAG; > + } > + > goto drop; > } > > @@ -1874,6 +1880,11 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > > if (unlikely(headlen > skb_headlen(skb))) { > atomic_long_inc(&tun->dev->rx_dropped); > + if (zerocopy) { > + skb_shinfo(skb)->destructor_arg = NULL; > + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY; > + skb_shinfo(skb)->tx_flags &= ~SKBTX_SHARED_FRAG; > + } > napi_free_frags(&tfile->napi); > rcu_read_unlock(); > mutex_unlock(&tfile->napi_mutex); It looks to me then we miss the failure feedback. The issues comes from the inconsistent error handling in tun. I wonder whether we can simply do uarg->callback(uarg, false) if necessary on every failture path on tun_get_user(). Note that, zerocopy has a lot of issues which makes it not good for production environment. Thanks
> -----Original Message----- > From: Jason Wang [mailto:jasowang@redhat.com] > Sent: Friday, December 4, 2020 2:11 PM > To: wangyunjian <wangyunjian@huawei.com>; mst@redhat.com > Cc: virtualization@lists.linux-foundation.org; netdev@vger.kernel.org; Lilijun > (Jerry) <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com> > Subject: Re: [PATCH net-next] tun: fix ubuf refcount incorrectly on error path > > > On 2020/12/3 下午4:00, wangyunjian 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 afterwards, the error > > handling in vhost handle_tx() will try to decrease the same refcount > > again. This is wrong and fix this by clearing ubuf_info when meeting > > 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> > > --- > > drivers/net/tun.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c index > > 2dc1988a8973..3614bb1b6d35 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -1861,6 +1861,12 @@ static ssize_t tun_get_user(struct tun_struct > *tun, struct tun_file *tfile, > > if (unlikely(!(tun->dev->flags & IFF_UP))) { > > err = -EIO; > > rcu_read_unlock(); > > + if (zerocopy) { > > + skb_shinfo(skb)->destructor_arg = NULL; > > + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY; > > + skb_shinfo(skb)->tx_flags &= ~SKBTX_SHARED_FRAG; > > + } > > + > > goto drop; > > } > > > > @@ -1874,6 +1880,11 @@ static ssize_t tun_get_user(struct tun_struct > > *tun, struct tun_file *tfile, > > > > if (unlikely(headlen > skb_headlen(skb))) { > > atomic_long_inc(&tun->dev->rx_dropped); > > + if (zerocopy) { > > + skb_shinfo(skb)->destructor_arg = NULL; > > + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY; > > + skb_shinfo(skb)->tx_flags &= ~SKBTX_SHARED_FRAG; > > + } > > napi_free_frags(&tfile->napi); > > rcu_read_unlock(); > > mutex_unlock(&tfile->napi_mutex); > > > It looks to me then we miss the failure feedback. > > The issues comes from the inconsistent error handling in tun. > > I wonder whether we can simply do uarg->callback(uarg, false) if necessary on > every failture path on tun_get_user(). How about this? --- drivers/net/tun.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 2dc1988a8973..36a8d8eacd7b 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1637,6 +1637,19 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun, return NULL; } +/* copy ubuf_info for callback when skb has no error */ +inline static 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 +1825,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 +1833,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); @@ -1880,7 +1884,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, WARN_ON(1); return -ENOMEM; } - + tun_copy_ubuf_info(skb, zerocopy, msg_control); local_bh_disable(); napi_gro_frags(&tfile->napi); local_bh_enable(); @@ -1889,6 +1893,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 +1904,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();
On 2020/12/4 下午6:22, wangyunjian wrote: >> -----Original Message----- >> From: Jason Wang [mailto:jasowang@redhat.com] >> Sent: Friday, December 4, 2020 2:11 PM >> To: wangyunjian <wangyunjian@huawei.com>; mst@redhat.com >> Cc: virtualization@lists.linux-foundation.org; netdev@vger.kernel.org; Lilijun >> (Jerry) <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com> >> Subject: Re: [PATCH net-next] tun: fix ubuf refcount incorrectly on error path >> >> >> On 2020/12/3 下午4:00, wangyunjian 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 afterwards, the error >>> handling in vhost handle_tx() will try to decrease the same refcount >>> again. This is wrong and fix this by clearing ubuf_info when meeting >>> 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> >>> --- >>> drivers/net/tun.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c index >>> 2dc1988a8973..3614bb1b6d35 100644 >>> --- a/drivers/net/tun.c >>> +++ b/drivers/net/tun.c >>> @@ -1861,6 +1861,12 @@ static ssize_t tun_get_user(struct tun_struct >> *tun, struct tun_file *tfile, >>> if (unlikely(!(tun->dev->flags & IFF_UP))) { >>> err = -EIO; >>> rcu_read_unlock(); >>> + if (zerocopy) { >>> + skb_shinfo(skb)->destructor_arg = NULL; >>> + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY; >>> + skb_shinfo(skb)->tx_flags &= ~SKBTX_SHARED_FRAG; >>> + } >>> + >>> goto drop; >>> } >>> >>> @@ -1874,6 +1880,11 @@ static ssize_t tun_get_user(struct tun_struct >>> *tun, struct tun_file *tfile, >>> >>> if (unlikely(headlen > skb_headlen(skb))) { >>> atomic_long_inc(&tun->dev->rx_dropped); >>> + if (zerocopy) { >>> + skb_shinfo(skb)->destructor_arg = NULL; >>> + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY; >>> + skb_shinfo(skb)->tx_flags &= ~SKBTX_SHARED_FRAG; >>> + } >>> napi_free_frags(&tfile->napi); >>> rcu_read_unlock(); >>> mutex_unlock(&tfile->napi_mutex); >> >> It looks to me then we miss the failure feedback. >> >> The issues comes from the inconsistent error handling in tun. >> >> I wonder whether we can simply do uarg->callback(uarg, false) if necessary on >> every failture path on tun_get_user(). > How about this? > > --- > drivers/net/tun.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 2dc1988a8973..36a8d8eacd7b 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1637,6 +1637,19 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun, > return NULL; > } > > +/* copy ubuf_info for callback when skb has no error */ > +inline static 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 +1825,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 +1833,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); If you think disabling zerocopy for XDP (which I think it makes sense). It's better to do this in another patch. > local_bh_disable(); > rcu_read_lock(); > xdp_prog = rcu_dereference(tun->xdp_prog); > @@ -1880,7 +1884,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > WARN_ON(1); > return -ENOMEM; > } > - > + tun_copy_ubuf_info(skb, zerocopy, msg_control); And for NAPI frags. > local_bh_disable(); > napi_gro_frags(&tfile->napi); > local_bh_enable(); > @@ -1889,6 +1893,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 +1904,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(); So it looks to me you want to disable zerocopy in all of the possible datapath? Thanks
> -----Original Message----- > From: Jason Wang [mailto:jasowang@redhat.com] > Sent: Monday, December 7, 2020 11:54 AM > To: wangyunjian <wangyunjian@huawei.com>; mst@redhat.com > Cc: virtualization@lists.linux-foundation.org; netdev@vger.kernel.org; Lilijun > (Jerry) <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com> > Subject: Re: [PATCH net-next] tun: fix ubuf refcount incorrectly on error path > > > On 2020/12/4 下午6:22, wangyunjian wrote: > >> -----Original Message----- > >> From: Jason Wang [mailto:jasowang@redhat.com] > >> Sent: Friday, December 4, 2020 2:11 PM > >> To: wangyunjian <wangyunjian@huawei.com>; mst@redhat.com > >> Cc: virtualization@lists.linux-foundation.org; netdev@vger.kernel.org; > Lilijun > >> (Jerry) <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com> > >> Subject: Re: [PATCH net-next] tun: fix ubuf refcount incorrectly on error path > >> > >> > >> On 2020/12/3 下午4:00, wangyunjian 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 afterwards, the error > >>> handling in vhost handle_tx() will try to decrease the same refcount > >>> again. This is wrong and fix this by clearing ubuf_info when meeting > >>> 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> > >>> --- > >>> drivers/net/tun.c | 11 +++++++++++ > >>> 1 file changed, 11 insertions(+) > >>> > >>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c index > >>> 2dc1988a8973..3614bb1b6d35 100644 > >>> --- a/drivers/net/tun.c > >>> +++ b/drivers/net/tun.c > >>> @@ -1861,6 +1861,12 @@ static ssize_t tun_get_user(struct tun_struct > >> *tun, struct tun_file *tfile, > >>> if (unlikely(!(tun->dev->flags & IFF_UP))) { > >>> err = -EIO; > >>> rcu_read_unlock(); > >>> + if (zerocopy) { > >>> + skb_shinfo(skb)->destructor_arg = NULL; > >>> + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY; > >>> + skb_shinfo(skb)->tx_flags &= ~SKBTX_SHARED_FRAG; > >>> + } > >>> + > >>> goto drop; > >>> } > >>> > >>> @@ -1874,6 +1880,11 @@ static ssize_t tun_get_user(struct tun_struct > >>> *tun, struct tun_file *tfile, > >>> > >>> if (unlikely(headlen > skb_headlen(skb))) { > >>> atomic_long_inc(&tun->dev->rx_dropped); > >>> + if (zerocopy) { > >>> + skb_shinfo(skb)->destructor_arg = NULL; > >>> + skb_shinfo(skb)->tx_flags &= > ~SKBTX_DEV_ZEROCOPY; > >>> + skb_shinfo(skb)->tx_flags &= ~SKBTX_SHARED_FRAG; > >>> + } > >>> napi_free_frags(&tfile->napi); > >>> rcu_read_unlock(); > >>> mutex_unlock(&tfile->napi_mutex); > >> > >> It looks to me then we miss the failure feedback. > >> > >> The issues comes from the inconsistent error handling in tun. > >> > >> I wonder whether we can simply do uarg->callback(uarg, false) if necessary > on > >> every failture path on tun_get_user(). > > How about this? > > > > --- > > drivers/net/tun.c | 29 ++++++++++++++++++----------- > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > index 2dc1988a8973..36a8d8eacd7b 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -1637,6 +1637,19 @@ static struct sk_buff *tun_build_skb(struct > tun_struct *tun, > > return NULL; > > } > > > > +/* copy ubuf_info for callback when skb has no error */ > > +inline static 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 +1825,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 +1833,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); > > > If you think disabling zerocopy for XDP (which I think it makes sense). > It's better to do this in another patch. > > > > local_bh_disable(); > > rcu_read_lock(); > > xdp_prog = rcu_dereference(tun->xdp_prog); > > @@ -1880,7 +1884,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, > struct tun_file *tfile, > > WARN_ON(1); > > return -ENOMEM; > > } > > - > > + tun_copy_ubuf_info(skb, zerocopy, msg_control); > > > And for NAPI frags. > > > > local_bh_disable(); > > napi_gro_frags(&tfile->napi); > > local_bh_enable(); > > @@ -1889,6 +1893,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 +1904,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(); > > > So it looks to me you want to disable zerocopy in all of the possible > datapath? I think the newly added code is easy to miss this problem, so I want to copy ubuf_info until we're sure there's no errors. Thanks, Yunjian > > Thanks
On 2020/12/7 下午9:38, wangyunjian wrote: > I think the newly added code is easy to miss this problem, so I want to > copy ubuf_info until we're sure there's no errors. > > Thanks, > Yunjian But isn't this actually a disabling of zerocopy? Thanks
On 2020/12/8 上午10:32, Jason Wang wrote: > > On 2020/12/7 下午9:38, wangyunjian wrote: >> I think the newly added code is easy to miss this problem, so I want to >> copy ubuf_info until we're sure there's no errors. >> >> Thanks, >> Yunjian > > > But isn't this actually a disabling of zerocopy? > > Thanks > > Sorry, I misread the patch. Please send a formal version, and let's move the discussion there. Thanks
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 2dc1988a8973..3614bb1b6d35 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1861,6 +1861,12 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, if (unlikely(!(tun->dev->flags & IFF_UP))) { err = -EIO; rcu_read_unlock(); + if (zerocopy) { + skb_shinfo(skb)->destructor_arg = NULL; + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY; + skb_shinfo(skb)->tx_flags &= ~SKBTX_SHARED_FRAG; + } + goto drop; } @@ -1874,6 +1880,11 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, if (unlikely(headlen > skb_headlen(skb))) { atomic_long_inc(&tun->dev->rx_dropped); + if (zerocopy) { + skb_shinfo(skb)->destructor_arg = NULL; + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY; + skb_shinfo(skb)->tx_flags &= ~SKBTX_SHARED_FRAG; + } napi_free_frags(&tfile->napi); rcu_read_unlock(); mutex_unlock(&tfile->napi_mutex);