Re: [PATCH net-next] vxlan: Return the correct errno code

From: Joe Perches
Date: Mon Jun 07 2021 - 10:50:26 EST


On Mon, 2021-06-07 at 22:44 +0800, Zheng Yongjun wrote:
> When kalloc or kmemdup failed, should return ENOMEM rather than ENOBUFS.

Why? Where in the call chain does it matter?
Have you inspected the entire call chain and their return value tests?

> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
[]
> @@ -711,11 +711,11 @@ static int vxlan_fdb_append(struct vxlan_fdb *f,
>  
>
>   rd = kmalloc(sizeof(*rd), GFP_ATOMIC);

And this should probably use kzalloc to avoid possible uninitialized members.

>   if (rd == NULL)
> - return -ENOBUFS;
> + return -ENOMEM;
>  
>   if (dst_cache_init(&rd->dst_cache, GFP_ATOMIC)) {
>   kfree(rd);
> - return -ENOBUFS;
> + return -ENOMEM;
>   }
>  
>   rd->remote_ip = *ip;

The struct is:

include/net/vxlan.h:struct vxlan_rdst {
include/net/vxlan.h- union vxlan_addr remote_ip;
include/net/vxlan.h- __be16 remote_port;
include/net/vxlan.h- u8 offloaded:1;
include/net/vxlan.h- __be32 remote_vni;
include/net/vxlan.h- u32 remote_ifindex;
include/net/vxlan.h- struct net_device *remote_dev;
include/net/vxlan.h- struct list_head list;
include/net/vxlan.h- struct rcu_head rcu;
include/net/vxlan.h- struct dst_cache dst_cache;
include/net/vxlan.h-};

And the code is:

if (dst_cache_init(&rd->dst_cache, GFP_ATOMIC)) {
kfree(rd);
return -ENOBUFS;
}

rd->remote_ip = *ip;
rd->remote_port = port;
rd->offloaded = false;
rd->remote_vni = vni;
rd->remote_ifindex = ifindex;

list_add_tail_rcu(&rd->list, &f->remotes);

*rdp = rd;

So it appears as if rd->remote_dev and rd->rcu are uninitialized.
I don't know if that matters, but it seems poor form.