Re: [PATCH 06/10] security: fix documentation for the path_chmod hook

[efremov]

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.