* securityfs_create_dir strange comment @ 2007-02-18 15:45 Jan Engelhardt 2007-02-20 21:18 ` Serge E. Hallyn 0 siblings, 1 reply; 8+ messages in thread From: Jan Engelhardt @ 2007-02-18 15:45 UTC (permalink / raw) To: Linux Kernel Mailing List 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 -- ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: securityfs_create_dir strange comment 2007-02-18 15:45 securityfs_create_dir strange comment Jan Engelhardt @ 2007-02-20 21:18 ` Serge E. Hallyn 2007-02-20 22:26 ` Greg KH 0 siblings, 1 reply; 8+ messages in thread From: Serge E. Hallyn @ 2007-02-20 21:18 UTC (permalink / raw) To: Jan Engelhardt, greg; +Cc: Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: securityfs_create_dir strange comment 2007-02-20 21:18 ` Serge E. Hallyn @ 2007-02-20 22:26 ` Greg KH 2007-02-20 23:45 ` Jan Engelhardt 0 siblings, 1 reply; 8+ messages in thread From: Greg KH @ 2007-02-20 22:26 UTC (permalink / raw) To: Serge E. Hallyn; +Cc: Jan Engelhardt, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: securityfs_create_dir strange comment 2007-02-20 22:26 ` Greg KH @ 2007-02-20 23:45 ` Jan Engelhardt 2007-02-21 4:05 ` Greg KH 0 siblings, 1 reply; 8+ messages in thread From: Jan Engelhardt @ 2007-02-20 23:45 UTC (permalink / raw) To: Greg KH; +Cc: Serge E. Hallyn, Linux Kernel Mailing List 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/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: securityfs_create_dir strange comment 2007-02-20 23:45 ` Jan Engelhardt @ 2007-02-21 4:05 ` Greg KH 2007-02-21 17:07 ` Jan Engelhardt 0 siblings, 1 reply; 8+ messages in thread From: Greg KH @ 2007-02-21 4:05 UTC (permalink / raw) To: Jan Engelhardt; +Cc: Serge E. Hallyn, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: securityfs_create_dir strange comment 2007-02-21 4:05 ` Greg KH @ 2007-02-21 17:07 ` Jan Engelhardt 2007-02-21 17:29 ` Greg KH 0 siblings, 1 reply; 8+ messages in thread From: Jan Engelhardt @ 2007-02-21 17:07 UTC (permalink / raw) To: Greg KH; +Cc: Serge E. Hallyn, Linux Kernel Mailing List 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 -- ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: securityfs_create_dir strange comment 2007-02-21 17:07 ` Jan Engelhardt @ 2007-02-21 17:29 ` Greg KH 2007-02-21 17:46 ` Jan Engelhardt 0 siblings, 1 reply; 8+ messages in thread From: Greg KH @ 2007-02-21 17:29 UTC (permalink / raw) To: Jan Engelhardt; +Cc: Serge E. Hallyn, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: securityfs_create_dir strange comment 2007-02-21 17:29 ` Greg KH @ 2007-02-21 17:46 ` Jan Engelhardt 0 siblings, 0 replies; 8+ messages in thread From: Jan Engelhardt @ 2007-02-21 17:46 UTC (permalink / raw) To: Greg KH; +Cc: Serge E. Hallyn, Linux Kernel Mailing List 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 -- ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2007-02-21 17:47 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-02-18 15:45 securityfs_create_dir strange comment Jan Engelhardt 2007-02-20 21:18 ` Serge E. Hallyn 2007-02-20 22:26 ` Greg KH 2007-02-20 23:45 ` Jan Engelhardt 2007-02-21 4:05 ` Greg KH 2007-02-21 17:07 ` Jan Engelhardt 2007-02-21 17:29 ` Greg KH 2007-02-21 17:46 ` Jan Engelhardt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.