netdev - RE: [PATCH net-next] netdevsim: correctly check return value of debugfs_create

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <014801d37884$3cc5d290$b65177b0$@lab.ntt.co.jp>
Date:   Tue, 19 Dec 2017 13:45:47 +0900
From:   "Prashant Bhole" <bhole_prashant_q7@....ntt.co.jp>
To:     "'Jakub Kicinski'" <jakub.kicinski@...ronome.com>,
        "'David Miller'" <davem@...emloft.net>
Cc:     <netdev@...r.kernel.org>
Subject: RE: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir


> From: Jakub Kicinski [mailto:jakub.kicinski@...ronome.com]
> 
> On Mon, 11 Dec 2017 13:46:48 +0900, Prashant Bhole wrote:
> > > From: David Miller [mailto:davem@...emloft.net]
> > >
> > > From: Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
> > > Date: Fri,  8 Dec 2017 09:52:50 +0900
> > >
> > > > Return value is now checked with IS_ERROR_OR_NULL because
> > > > debugfs_create_dir doesn't return error value. It either returns
> > > > NULL or a valid pointer.
> > > >
> > > > Signed-off-by: Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
> > > > ---
> > > >  drivers/net/netdevsim/netdev.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/netdevsim/netdev.c
> > > > b/drivers/net/netdevsim/netdev.c index eb8c679fca9f..88d8ee2c89da
> > > > 100644
> > > > --- a/drivers/net/netdevsim/netdev.c
> > > > +++ b/drivers/net/netdevsim/netdev.c
> > > > @@ -469,7 +469,7 @@ static int __init nsim_module_init(void)
> > > >  	int err;
> > > >
> > > >  	nsim_ddir = debugfs_create_dir(DRV_NAME, NULL);
> > > > -	if (IS_ERR(nsim_ddir))
> > > > +	if (IS_ERR_OR_NULL(nsim_ddir))
> > > >  		return PTR_ERR(nsim_ddir);
> > >
> > > debugfs_create_dir() should really be fixed, either it uses error
> > > pointers consistently and therefore always provides a suitable error
> > > code to return
> > or it
> > > always uses NULL.
> > >
> > > This in-between behavior makes using it as an interface painful
> > > because no
> > clear
> > > meaning is given to NULL.
> > >
> > > So please do the work necessary to make debugfs_create_dir()'s
> > > return semantics clearer and more useful.
> > >
> > > Thank you.
> >
> > Dave,
> > Thanks for comments. I will try to fix error handling in netdevsim
first.
> >
> > Jakub,
> > Let's decide with an example. The typical directory structure for
> > netdevsim interface is as below:
> > /sys/kernel/debug/netdevsim/sim0/bpf_bound_progs/
> > Please let me know if you are ok with following:
> >
> > 1) If debugfs_create_dir() fails in module_init, let's keep it fatal
> > error with corrected condition:
> > +	if (IS_ERR_OR_NULL(nsim_ddir))
> > +		return -ENOMEM;
> >
> > 2) In case sim0 or bpf_bound_progs are  fail to create, we need to add
> > checks before creating any file in them.
> 
> Fine with me, although if you fix DebugFS first you could use the real
error from
> the start here.

Jakub, Dave,
Sorry for late reply.
I tried to evaluate whether fixing return value of debugfs_create_dir() (and
friends) will be useful or not because it has not been changed since very
long time. Now I am not much convinced about changing this api. 

Important and possible error codes could be -EEXIST and -ENOMEM. Suppose
-EEXIST is returned, IMO the directory shouldn't exists in the first place
because it is specific to particular module. Also, there is no point in
creating file in such directory, because directory owner (creator) might
remove it too. This means there are less chances that api change will be
useful. Please let me know your opinion on it.

If you are ok with above explanation, shall I submit v2 for this patch?

-Prashant