| Message ID | 0275d06334cdb1d2a87384d7971924a70776b3cb.1549540487.git.efremov@ispras.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LSM documentation update | expand |
On Thu, Feb 07, 2019 at 03:44:54PM +0300, Denis Efremov wrote: > The path_chmod hook was changed in the commit > "switch security_path_chmod() to struct path *" (cdcf116d44e7). > The argument @mnt was removed from the hook, @dentry was changed > to @path. This patch updates the documentation accordingly. > > Signed-off-by: Denis Efremov <efremov@ispras.ru> > --- > include/linux/lsm_hooks.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index cb93972257be..5d6428d0027b 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -304,8 +304,7 @@ > * Return 0 if permission is granted. > * @path_chmod: > * Check for permission to change DAC's permission of a file or directory. > - * @dentry contains the dentry structure. > - * @mnt contains the vfsmnt structure. > + * @path contains the path structure. May I politely inquire about the value of these comments? How much information is provided by refering to an argument as "the dentry structure" or "the path structure", especially when there's nothing immediately above that would introduce either. "Type of 'dentry' argument is somehow related to struct dentry, try and guess what the value might be - we don't care, we just need every argument commented"? Who needs that crap in the first place?
On Thursday, February 07, 2019 8:50 AM Al Viro wrote: > On Thu, Feb 07, 2019 at 03:44:54PM +0300, Denis Efremov wrote: > > The path_chmod hook was changed in the commit > > "switch security_path_chmod() to struct path *" (cdcf116d44e7). > > The argument @mnt was removed from the hook, @dentry was changed > > to @path. This patch updates the documentation accordingly. > > > > Signed-off-by: Denis Efremov <efremov@ispras.ru> > > --- > > include/linux/lsm_hooks.h | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > > index cb93972257be..5d6428d0027b 100644 > > --- a/include/linux/lsm_hooks.h > > +++ b/include/linux/lsm_hooks.h > > @@ -304,8 +304,7 @@ > > * Return 0 if permission is granted. > > * @path_chmod: > > * Check for permission to change DAC's permission of a file or directory. > > - * @dentry contains the dentry structure. > > - * @mnt contains the vfsmnt structure. > > + * @path contains the path structure. > > May I politely inquire about the value of these comments? How much information > is provided by refering to an argument as "the dentry structure" or "the path > structure", especially when there's nothing immediately above that would introduce > either. "Type of 'dentry' argument is somehow related to struct dentry, > try and guess what the value might be - we don't care, we just need every > argument commented"? > > Who needs that crap in the first place? The comments fill a valuable place to folks like me who are new to the linux security modules. In my spare time, I'm writing a new LSM specifically geared for parental controls uses, and the comments in lsm_hooks.h have helped me out more than once. Perhaps the comments could be inproved by changing them to something like this: "@[arg] contains the [type] structure, defined in linux/[?].h"
On 2/7/19 9:09 AM, Edwin Zimmerman wrote: > On Thursday, February 07, 2019 8:50 AM Al Viro wrote: >> On Thu, Feb 07, 2019 at 03:44:54PM +0300, Denis Efremov wrote: >>> The path_chmod hook was changed in the commit >>> "switch security_path_chmod() to struct path *" (cdcf116d44e7). >>> The argument @mnt was removed from the hook, @dentry was changed >>> to @path. This patch updates the documentation accordingly. >>> >>> Signed-off-by: Denis Efremov <efremov@ispras.ru> >>> --- >>> include/linux/lsm_hooks.h | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>> index cb93972257be..5d6428d0027b 100644 >>> --- a/include/linux/lsm_hooks.h >>> +++ b/include/linux/lsm_hooks.h >>> @@ -304,8 +304,7 @@ >>> * Return 0 if permission is granted. >>> * @path_chmod: >>> * Check for permission to change DAC's permission of a file or directory. >>> - * @dentry contains the dentry structure. >>> - * @mnt contains the vfsmnt structure. >>> + * @path contains the path structure. >> >> May I politely inquire about the value of these comments? How much information >> is provided by refering to an argument as "the dentry structure" or "the path >> structure", especially when there's nothing immediately above that would introduce >> either. "Type of 'dentry' argument is somehow related to struct dentry, >> try and guess what the value might be - we don't care, we just need every >> argument commented"? >> >> Who needs that crap in the first place? > > The comments fill a valuable place to folks like me who are new to the linux security modules. > In my spare time, I'm writing a new LSM specifically geared for parental controls uses, and the > comments in lsm_hooks.h have helped me out more than once. Perhaps the comments could > be inproved by changing them to something like this: > "@[arg] contains the [type] structure, defined in linux/[?].h" I don't think so. The point is not what type of structure but what object is being passed and why is it relevant to the hook, e.g. + @path contains the path structure for the file whose permissions are being modified or similar.
On Thu, Feb 07, 2019 at 09:09:49AM -0500, Edwin Zimmerman wrote: > > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > > > index cb93972257be..5d6428d0027b 100644 > > > --- a/include/linux/lsm_hooks.h > > > +++ b/include/linux/lsm_hooks.h > > > @@ -304,8 +304,7 @@ > > > * Return 0 if permission is granted. > > > * @path_chmod: > > > * Check for permission to change DAC's permission of a file or directory. > > > - * @dentry contains the dentry structure. > > > - * @mnt contains the vfsmnt structure. > > > + * @path contains the path structure. > > > > May I politely inquire about the value of these comments? How much information > > is provided by refering to an argument as "the dentry structure" or "the path > > structure", especially when there's nothing immediately above that would introduce > > either. "Type of 'dentry' argument is somehow related to struct dentry, > > try and guess what the value might be - we don't care, we just need every > > argument commented"? > > > > Who needs that crap in the first place? > > The comments fill a valuable place to folks like me who are new to the linux security modules. > In my spare time, I'm writing a new LSM specifically geared for parental controls uses, and the > comments in lsm_hooks.h have helped me out more than once. Perhaps the comments could > be inproved by changing them to something like this: > "@[arg] contains the [type] structure, defined in linux/[?].h" Um... The _type_ of argument is visible in declaration already; it doesn't say a damn thing about the value of that argument. In this particular case, dentry/mnt pair (whichever way it gets passed; struct path is exactly such a pair) is actually used to specify the location of file or directory in question, but try to guess that by description given in this "documentation"... As for "defined in"... that's what grep/ctags/etc. are for. Again, the useful information about an argument is _what_ gets passed in it, not just which type it is...
On 2/7/19 9:32 AM, Stephen Smalley wrote: > On 2/7/19 9:09 AM, Edwin Zimmerman wrote: >> On Thursday, February 07, 2019 8:50 AM Al Viro wrote: >>> On Thu, Feb 07, 2019 at 03:44:54PM +0300, Denis Efremov wrote: >>>> The path_chmod hook was changed in the commit >>>> "switch security_path_chmod() to struct path *" (cdcf116d44e7). >>>> The argument @mnt was removed from the hook, @dentry was changed >>>> to @path. This patch updates the documentation accordingly. >>>> >>>> Signed-off-by: Denis Efremov <efremov@ispras.ru> >>>> --- >>>> include/linux/lsm_hooks.h | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>>> index cb93972257be..5d6428d0027b 100644 >>>> --- a/include/linux/lsm_hooks.h >>>> +++ b/include/linux/lsm_hooks.h >>>> @@ -304,8 +304,7 @@ >>>> * Return 0 if permission is granted. >>>> * @path_chmod: >>>> * Check for permission to change DAC's permission of a file or >>>> directory. >>>> - * @dentry contains the dentry structure. >>>> - * @mnt contains the vfsmnt structure. >>>> + * @path contains the path structure. >>> >>> May I politely inquire about the value of these comments? How much >>> information >>> is provided by refering to an argument as "the dentry structure" or >>> "the path >>> structure", especially when there's nothing immediately above that >>> would introduce >>> either. "Type of 'dentry' argument is somehow related to struct dentry, >>> try and guess what the value might be - we don't care, we just need >>> every >>> argument commented"? >>> >>> Who needs that crap in the first place? >> >> The comments fill a valuable place to folks like me who are new to the >> linux security modules. >> In my spare time, I'm writing a new LSM specifically geared for >> parental controls uses, and the >> comments in lsm_hooks.h have helped me out more than once. Perhaps >> the comments could >> be inproved by changing them to something like this: >> "@[arg] contains the [type] structure, defined in linux/[?].h" > > I don't think so. The point is not what type of structure but what > object is being passed and why is it relevant to the hook, e.g. > > + @path contains the path structure for the file whose permissions are > being modified > > or similar. It would probably be better to amend the description too to refer to the argument in context, e.g. * @path_chmod: * Check for permission to change the mode of the file referenced by @path. * @path the file whose mode would be modified or similar. I'd suggest looking to kerneldoc comments in fs/*.c or elsewhere as better examples.
Al Viro писал 2019-02-07 17:46: > On Thu, Feb 07, 2019 at 09:09:49AM -0500, Edwin Zimmerman wrote: >> > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> > > index cb93972257be..5d6428d0027b 100644 >> > > --- a/include/linux/lsm_hooks.h >> > > +++ b/include/linux/lsm_hooks.h >> > > @@ -304,8 +304,7 @@ >> > > * Return 0 if permission is granted. >> > > * @path_chmod: >> > > * Check for permission to change DAC's permission of a file or directory. >> > > - * @dentry contains the dentry structure. >> > > - * @mnt contains the vfsmnt structure. >> > > + * @path contains the path structure. >> > >> > May I politely inquire about the value of these comments? How much information >> > is provided by refering to an argument as "the dentry structure" or "the path >> > structure", especially when there's nothing immediately above that would introduce >> > either. "Type of 'dentry' argument is somehow related to struct dentry, >> > try and guess what the value might be - we don't care, we just need every >> > argument commented"? >> > >> > Who needs that crap in the first place? >> >> The comments fill a valuable place to folks like me who are new to the >> linux security modules. >> In my spare time, I'm writing a new LSM specifically geared for >> parental controls uses, and the >> comments in lsm_hooks.h have helped me out more than once. Perhaps >> the comments could >> be inproved by changing them to something like this: >> "@[arg] contains the [type] structure, defined in linux/[?].h" > > Um... The _type_ of argument is visible in declaration already; > it doesn't say a damn thing about the value of that argument. > > In this particular case, dentry/mnt pair (whichever way it gets > passed; struct path is exactly such a pair) is actually used to > specify the location of file or directory in question, but > try to guess that by description given in this "documentation"... > > As for "defined in"... that's what grep/ctags/etc. are for. > Again, the useful information about an argument is _what_ gets > passed in it, not just which type it is... While I completely agree about the doubtful value of such comments, the whole documentation is written in style like that. Unfortunately, I don't have the knowledge to rewrite the documentation in every case even for functions from this patchset. And I will be surprised if a single person could do it for the whole LSM set. The patchset only minimally updates the documentation to lift it up to the current LSM declarations. And maybe to show once again that despite we've got the documentation on a hook it maybe not actual for 12 years since the hook was changed in 2006. But of course, it doesn't mean that documentation is not needed. This can give the maintainers additional arguments for stricter checks before accepting new hooks and changing the existing ones. I will try to improve the description in the second version of the patchset.
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index cb93972257be..5d6428d0027b 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -304,8 +304,7 @@ * Return 0 if permission is granted. * @path_chmod: * Check for permission to change DAC's permission of a file or directory. - * @dentry contains the dentry structure. - * @mnt contains the vfsmnt structure. + * @path contains the path structure. * @mode contains DAC's mode. * Return 0 if permission is granted. * @path_chown:
The path_chmod hook was changed in the commit "switch security_path_chmod() to struct path *" (cdcf116d44e7). The argument @mnt was removed from the hook, @dentry was changed to @path. This patch updates the documentation accordingly. Signed-off-by: Denis Efremov <efremov@ispras.ru> --- include/linux/lsm_hooks.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)