[06/10] security: fix documentation for the path_chmod hook
diff mbox series

Message ID 0275d06334cdb1d2a87384d7971924a70776b3cb.1549540487.git.efremov@ispras.ru
State New
Headers show
Series
  • LSM documentation update
Related show

Commit Message

Denis Efremov Feb. 7, 2019, 12:44 p.m. UTC
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(-)

Comments

Al Viro Feb. 7, 2019, 1:49 p.m. UTC | #1
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?
Edwin Zimmerman Feb. 7, 2019, 2:09 p.m. UTC | #2
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"
Stephen Smalley Feb. 7, 2019, 2:32 p.m. UTC | #3
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.
Al Viro Feb. 7, 2019, 2:46 p.m. UTC | #4
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...
Stephen Smalley Feb. 7, 2019, 2:55 p.m. UTC | #5
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.
Denis Efremov Feb. 17, 2019, 6:45 p.m. UTC | #6
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.

Patch
diff mbox series

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: