| Message ID | 1608810533-8308-1-git-send-email-wangyunjian@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] tun: fix return value when the number of iovs exceeds MAX_SKB_FRAGS | 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/cc_maintainers | fail | 4 maintainers not CCed: maheshb@google,com davem@davemloft.net peterpenkov96@gmail.com kuba@kernel.org |
| 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 | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| 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 Thu, Dec 24, 2020 at 6:51 AM wangyunjian <wangyunjian@huawei.com> wrote: > > From: Yunjian Wang <wangyunjian@huawei.com> > > Currently the tun_napi_alloc_frags() function returns -ENOMEM when the > number of iovs exceeds MAX_SKB_FRAGS + 1. However this is inappropriate, > we should use -EMSGSIZE instead of -ENOMEM. > > Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver") > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> Acked-by: Willem de Bruijn <willemb@google.com> It might be good to explain why the distinction matters: one denotes a transient failure that the caller (specifically vhost_net) can retry, the other a persistent failure due to bad packet geometry that should be dropped.
On Thu, Dec 24, 2020 at 07:48:53PM +0800, wangyunjian wrote: > From: Yunjian Wang <wangyunjian@huawei.com> > > Currently the tun_napi_alloc_frags() function returns -ENOMEM when the > number of iovs exceeds MAX_SKB_FRAGS + 1. However this is inappropriate, > we should use -EMSGSIZE instead of -ENOMEM. > > Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver") > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > drivers/net/tun.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 2dc1988a8973..15c6dd7fb04c 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1365,7 +1365,7 @@ static struct sk_buff *tun_napi_alloc_frags(struct tun_file *tfile, > int i; > > if (it->nr_segs > MAX_SKB_FRAGS + 1) > - return ERR_PTR(-ENOMEM); > + return ERR_PTR(-EMSGSIZE); > > local_bh_disable(); > skb = napi_get_frags(&tfile->napi); > -- > 2.23.0
On Thu, Dec 24, 2020 at 10:56:16AM -0500, Willem de Bruijn wrote: > On Thu, Dec 24, 2020 at 6:51 AM wangyunjian <wangyunjian@huawei.com> wrote: > > > > From: Yunjian Wang <wangyunjian@huawei.com> > > > > Currently the tun_napi_alloc_frags() function returns -ENOMEM when the > > number of iovs exceeds MAX_SKB_FRAGS + 1. However this is inappropriate, > > we should use -EMSGSIZE instead of -ENOMEM. > > > > Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver") > > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > > Acked-by: Willem de Bruijn <willemb@google.com> > > It might be good to explain why the distinction matters: one denotes a > transient failure that the caller (specifically vhost_net) can retry, > the other a persistent failure due to bad packet geometry that should > be dropped. It would be good to have a definition of what constitutes a transient failure. At the moment there's a proposed patch to vhost that tests for err == -EAGAIN || err == -ENOMEM || err == -ENOBUFS and we'll likely add EIO to that. Alternatively, I'm inclined to say any invalid input should just return EINVAL except maybe for memory access errors which traditionally are EFAULT. Then vhost can handle any failure except EINVAL and EFAULT as transient.
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 2dc1988a8973..15c6dd7fb04c 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1365,7 +1365,7 @@ static struct sk_buff *tun_napi_alloc_frags(struct tun_file *tfile, int i; if (it->nr_segs > MAX_SKB_FRAGS + 1) - return ERR_PTR(-ENOMEM); + return ERR_PTR(-EMSGSIZE); local_bh_disable(); skb = napi_get_frags(&tfile->napi);