securityfs_create_dir strange comment

securityfs_create_dir strange comment

[Jan Engelhardt]

Hello list,


in security/inode.c, the comment for securityfs_create_dir() reads:

	If securityfs is not enabled in the kernel, the value -ENODEV 
	will be returned.  It is not wise to check for this value, but 
	rather, check for NULL or !NULL instead as to eliminate the need 
	for #ifdef in the calling code.

What is the actual callee that can return NULL - and what should 
module_init() of a module return when securityfs_create_dir() returns 
NULL?


Thanks,
Jan
-- 

Re: securityfs_create_dir strange comment

[Serge E. Hallyn]

Quoting Jan Engelhardt (jengelh@linux01.gwdg.de):
> Hello list,
> 
> 
> in security/inode.c, the comment for securityfs_create_dir() reads:
> 
> 	If securityfs is not enabled in the kernel, the value -ENODEV 
> 	will be returned.  It is not wise to check for this value, but 
> 	rather, check for NULL or !NULL instead as to eliminate the need 
> 	for #ifdef in the calling code.
> 
> What is the actual callee that can return NULL - and what should 
> module_init() of a module return when securityfs_create_dir() returns 
> NULL?

Hmm, this came from GregKH.  It does seem based on the code that
checking for -ENODEV is necessary, so I don't understand the comment.

thanks,
-serge

Re: securityfs_create_dir strange comment

[Greg KH]

On Tue, Feb 20, 2007 at 03:18:49PM -0600, Serge E. Hallyn wrote:
> Quoting Jan Engelhardt (jengelh@linux01.gwdg.de):
> > Hello list,
> > 
> > 
> > in security/inode.c, the comment for securityfs_create_dir() reads:
> > 
> > 	If securityfs is not enabled in the kernel, the value -ENODEV 
> > 	will be returned.  It is not wise to check for this value, but 
> > 	rather, check for NULL or !NULL instead as to eliminate the need 
> > 	for #ifdef in the calling code.
> > 
> > What is the actual callee that can return NULL - and what should 
> > module_init() of a module return when securityfs_create_dir() returns 
> > NULL?
> 
> Hmm, this came from GregKH.  It does seem based on the code that
> checking for -ENODEV is necessary, so I don't understand the comment.

If securityfs_create_dir() returns NULL, then something bad happened and
your code needs to properly recover from it.

Other than that, I don't understand the issue here.

confused,

greg k-h

Re: securityfs_create_dir strange comment

[Jan Engelhardt]

On Feb 20 2007 14:26, Greg KH wrote:
>On Tue, Feb 20, 2007 at 03:18:49PM -0600, Serge E. Hallyn wrote:
>> Quoting Jan Engelhardt (jengelh@linux01.gwdg.de):
>> > Hello list,
>> > 
>> > 
>> > in security/inode.c, the comment for securityfs_create_dir() reads:
>> > 
>> > 	If securityfs is not enabled in the kernel, the value -ENODEV 
>> > 	will be returned.  It is not wise to check for this value, but 
>> > 	rather, check for NULL or !NULL instead as to eliminate the need 
>> > 	for #ifdef in the calling code.
>> > 
>> > What is the actual callee that can return NULL - and what should 
>> > module_init() of a module return when securityfs_create_dir() returns 
>> > NULL?
>> 
>> Hmm, this came from GregKH.  It does seem based on the code that
>> checking for -ENODEV is necessary, so I don't understand the comment.
>
>If securityfs_create_dir() returns NULL, then something bad happened and
>your code needs to properly recover from it.
>
>Other than that, I don't understand the issue here.

Consider:

static __init int mymodule_init(void)
{
    struct dentry *de;
    de = securityfs_create_dir("foobar", NULL);

    /* case 1 */
    if(IS_ERR(de))
        return PTR_ERR(de);

    /* case 2 */
    if(de == NULL)
        return WHAT_HERE; /* -EIO? */
}

There are two error cases. One: when the function gives us an error code.
Two: When it returns NULL, without an error code. This looks bogus to me.
What error is it, when there is no error? - And what should I return to
modprobe in that case?


Jan
-- 
ft: http://freshmeat.net/p/chaostables/

Re: securityfs_create_dir strange comment

[Greg KH]

On Wed, Feb 21, 2007 at 12:45:40AM +0100, Jan Engelhardt wrote:
> 
> On Feb 20 2007 14:26, Greg KH wrote:
> >On Tue, Feb 20, 2007 at 03:18:49PM -0600, Serge E. Hallyn wrote:
> >> Quoting Jan Engelhardt (jengelh@linux01.gwdg.de):
> >> > Hello list,
> >> > 
> >> > 
> >> > in security/inode.c, the comment for securityfs_create_dir() reads:
> >> > 
> >> > 	If securityfs is not enabled in the kernel, the value -ENODEV 
> >> > 	will be returned.  It is not wise to check for this value, but 
> >> > 	rather, check for NULL or !NULL instead as to eliminate the need 
> >> > 	for #ifdef in the calling code.
> >> > 
> >> > What is the actual callee that can return NULL - and what should 
> >> > module_init() of a module return when securityfs_create_dir() returns 
> >> > NULL?
> >> 
> >> Hmm, this came from GregKH.  It does seem based on the code that
> >> checking for -ENODEV is necessary, so I don't understand the comment.
> >
> >If securityfs_create_dir() returns NULL, then something bad happened and
> >your code needs to properly recover from it.
> >
> >Other than that, I don't understand the issue here.
> 
> Consider:
> 
> static __init int mymodule_init(void)
> {
>     struct dentry *de;
>     de = securityfs_create_dir("foobar", NULL);
> 
>     /* case 1 */
>     if(IS_ERR(de))
>         return PTR_ERR(de);
> 
>     /* case 2 */
>     if(de == NULL)
>         return WHAT_HERE; /* -EIO? */
> }
> 
> There are two error cases. One: when the function gives us an error code.
> Two: When it returns NULL, without an error code. This looks bogus to me.
> What error is it, when there is no error? - And what should I return to
> modprobe in that case?

Try this instead:
	if (!de)
		return -ENOMEM;
	if ((IS_ERR(de)) && (PTR_ERR(de) != -ENODEV))
		return PTR_ERR(de);
	return 0;

That should cover everything properly, right?

thanks,

greg k-h

Re: securityfs_create_dir strange comment

[Jan Engelhardt]

Hello Greg,


On Feb 20 2007 20:05, Greg KH wrote:
>
>Try this instead:
>	if (!de)
>		return -ENOMEM;
>	if ((IS_ERR(de)) && (PTR_ERR(de) != -ENODEV))
>		return PTR_ERR(de);
>	return 0;
>
>That should cover everything properly, right?

In case memory could not be allocated, why does not securityfs_*() return
ERR_PTR(-ENOMEM) then? (I think, that's the quintessential question after
all. And thanks for giving an example what to do in the ENODEV case.)


Jan
-- 

Re: securityfs_create_dir strange comment

[Greg KH]

On Wed, Feb 21, 2007 at 06:07:56PM +0100, Jan Engelhardt wrote:
> Hello Greg,
> 
> 
> On Feb 20 2007 20:05, Greg KH wrote:
> >
> >Try this instead:
> >	if (!de)
> >		return -ENOMEM;
> >	if ((IS_ERR(de)) && (PTR_ERR(de) != -ENODEV))
> >		return PTR_ERR(de);
> >	return 0;
> >
> >That should cover everything properly, right?
> 
> In case memory could not be allocated, why does not securityfs_*() return
> ERR_PTR(-ENOMEM) then? (I think, that's the quintessential question after
> all. And thanks for giving an example what to do in the ENODEV case.)

Actually, in reading the code (which might have helped in the first
place), we can never return NULL if securityfs is enabled.  So you can
just drop that first check entirely.

Which makes me wonder, it might be easier to just return NULL if
securityfs is not enabled in the kernel, as long as no one checks that
improperly...

Hope this helps,

greg k-h

Re: securityfs_create_dir strange comment

[Jan Engelhardt]

Hi Greg,

>> >Try this instead:
>> >	if (!de)
>> >		return -ENOMEM;
>> >	if ((IS_ERR(de)) && (PTR_ERR(de) != -ENODEV))
>> >		return PTR_ERR(de);
>> >	return 0;
>> >
>> >That should cover everything properly, right?
>> 
>> In case memory could not be allocated, why does not securityfs_*() return
>> ERR_PTR(-ENOMEM) then? (I think, that's the quintessential question after
>> all. And thanks for giving an example what to do in the ENODEV case.)
>
>Actually, in reading the code (which might have helped in the first
>place), we can never return NULL if securityfs is enabled.

So we're back to the confusing comment ;-)

>So you can just drop that first check entirely.
>
>Which makes me wonder, it might be easier to just return NULL if
>securityfs is not enabled in the kernel, as long as no one checks that
>improperly...

I have actually had a look into the tree who even uses securityfs.
The most prominent case are LSMs. They need CONFIG_SECURITY=y anyway,
so securityfs is always enabled for those. What remains seems to be
tpm_bios.c.


Jan
--