1. Patchew
  2. linux
  3. View series

[PATCH] security: Fix ret values doc for security_inode_init_security()

Roberto Sassu posted 1 patch 2 years, 6 months ago
There is a newer version of this series
security/security.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] security: Fix ret values doc for security_inode_init_security()
Posted by Roberto Sassu 2 years, 6 months ago 
From: Roberto Sassu <roberto.sassu@huawei.com>

Commit 6bcdfd2cac55 ("security: Allow all LSMs to provide xattrs for
inode_init_security hook") unified the !initxattrs and initxattrs cases. By
doing that, security_inode_init_security() cannot return -EOPNOTSUPP
anymore, as it is always replaced with zero at the end of the function.

Also, mentioning -ENOMEM as the only possible error is not correct. For
example, evm_inode_init_security() could return -ENOKEY.

Fix these issues in the documentation of security_inode_init_security().

Fixes: 6bcdfd2cac55 ("security: Allow all LSMs to provide xattrs for inode_init_security hook")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/security.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/security.c b/security/security.c
index cfdd0cbbcb9..5aa9cb91f0f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1604,8 +1604,8 @@ EXPORT_SYMBOL(security_dentry_create_files_as);
  * a security attribute on this particular inode, then it should return
  * -EOPNOTSUPP to skip this processing.
  *
- * Return: Returns 0 on success, -EOPNOTSUPP if no security attribute is
- * needed, or -ENOMEM on memory allocation failure.
+ * Return: Returns 0 on success or on -EOPNOTSUPP error, a negative value other
+ *         than -EOPNOTSUPP otherwise.
  */
 int security_inode_init_security(struct inode *inode, struct inode *dir,
 				 const struct qstr *qstr,
-- 
2.34.1
Re: [PATCH] security: Fix ret values doc for security_inode_init_security()
Posted by Paul Moore 2 years, 6 months ago 
On Mon, Jul 24, 2023 at 10:52 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Commit 6bcdfd2cac55 ("security: Allow all LSMs to provide xattrs for
> inode_init_security hook") unified the !initxattrs and initxattrs cases. By
> doing that, security_inode_init_security() cannot return -EOPNOTSUPP
> anymore, as it is always replaced with zero at the end of the function.
>
> Also, mentioning -ENOMEM as the only possible error is not correct. For
> example, evm_inode_init_security() could return -ENOKEY.
>
> Fix these issues in the documentation of security_inode_init_security().
>
> Fixes: 6bcdfd2cac55 ("security: Allow all LSMs to provide xattrs for inode_init_security hook")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/security.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index cfdd0cbbcb9..5aa9cb91f0f 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1604,8 +1604,8 @@ EXPORT_SYMBOL(security_dentry_create_files_as);
>   * a security attribute on this particular inode, then it should return
>   * -EOPNOTSUPP to skip this processing.
>   *
> - * Return: Returns 0 on success, -EOPNOTSUPP if no security attribute is
> - * needed, or -ENOMEM on memory allocation failure.
> + * Return: Returns 0 on success or on -EOPNOTSUPP error, a negative value other
> + *         than -EOPNOTSUPP otherwise.

How about "Returns 0 if the LSM successfully initialized all of the
inode security attributes that are required, negative values
otherwise."?  The caller doesn't need to worry about the individual
LSMs returning -EOPNOTSUPP in the case of no security attributes, and
if they really care, they are likely reading the description above (or
the code) which explains it in much better detail.

Thoughts?

--
paul-moore.com
Re: [PATCH] security: Fix ret values doc for security_inode_init_security()
Posted by Roberto Sassu 2 years, 6 months ago 
On Mon, 2023-07-24 at 17:54 -0400, Paul Moore wrote:
> On Mon, Jul 24, 2023 at 10:52 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > 
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Commit 6bcdfd2cac55 ("security: Allow all LSMs to provide xattrs for
> > inode_init_security hook") unified the !initxattrs and initxattrs cases. By
> > doing that, security_inode_init_security() cannot return -EOPNOTSUPP
> > anymore, as it is always replaced with zero at the end of the function.
> > 
> > Also, mentioning -ENOMEM as the only possible error is not correct. For
> > example, evm_inode_init_security() could return -ENOKEY.
> > 
> > Fix these issues in the documentation of security_inode_init_security().
> > 
> > Fixes: 6bcdfd2cac55 ("security: Allow all LSMs to provide xattrs for inode_init_security hook")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  security/security.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/security/security.c b/security/security.c
> > index cfdd0cbbcb9..5aa9cb91f0f 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1604,8 +1604,8 @@ EXPORT_SYMBOL(security_dentry_create_files_as);
> >   * a security attribute on this particular inode, then it should return
> >   * -EOPNOTSUPP to skip this processing.
> >   *
> > - * Return: Returns 0 on success, -EOPNOTSUPP if no security attribute is
> > - * needed, or -ENOMEM on memory allocation failure.
> > + * Return: Returns 0 on success or on -EOPNOTSUPP error, a negative value other
> > + *         than -EOPNOTSUPP otherwise.
> 
> How about "Returns 0 if the LSM successfully initialized all of the
> inode security attributes that are required, negative values
> otherwise."?  The caller doesn't need to worry about the individual
> LSMs returning -EOPNOTSUPP in the case of no security attributes, and
> if they really care, they are likely reading the description above (or
> the code) which explains it in much better detail.

Maybe this could be better:

Return 0 if security attributes initialization is successful or not
necessary, a negative value otherwise.

Thanks

Roberto
Re: [PATCH] security: Fix ret values doc for security_inode_init_security()
Posted by Paul Moore 2 years, 6 months ago 
On Tue, Jul 25, 2023 at 3:02 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Mon, 2023-07-24 at 17:54 -0400, Paul Moore wrote:
> > On Mon, Jul 24, 2023 at 10:52 AM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > >
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > Commit 6bcdfd2cac55 ("security: Allow all LSMs to provide xattrs for
> > > inode_init_security hook") unified the !initxattrs and initxattrs cases. By
> > > doing that, security_inode_init_security() cannot return -EOPNOTSUPP
> > > anymore, as it is always replaced with zero at the end of the function.
> > >
> > > Also, mentioning -ENOMEM as the only possible error is not correct. For
> > > example, evm_inode_init_security() could return -ENOKEY.
> > >
> > > Fix these issues in the documentation of security_inode_init_security().
> > >
> > > Fixes: 6bcdfd2cac55 ("security: Allow all LSMs to provide xattrs for inode_init_security hook")
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  security/security.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/security/security.c b/security/security.c
> > > index cfdd0cbbcb9..5aa9cb91f0f 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -1604,8 +1604,8 @@ EXPORT_SYMBOL(security_dentry_create_files_as);
> > >   * a security attribute on this particular inode, then it should return
> > >   * -EOPNOTSUPP to skip this processing.
> > >   *
> > > - * Return: Returns 0 on success, -EOPNOTSUPP if no security attribute is
> > > - * needed, or -ENOMEM on memory allocation failure.
> > > + * Return: Returns 0 on success or on -EOPNOTSUPP error, a negative value other
> > > + *         than -EOPNOTSUPP otherwise.
> >
> > How about "Returns 0 if the LSM successfully initialized all of the
> > inode security attributes that are required, negative values
> > otherwise."?  The caller doesn't need to worry about the individual
> > LSMs returning -EOPNOTSUPP in the case of no security attributes, and
> > if they really care, they are likely reading the description above (or
> > the code) which explains it in much better detail.
>
> Maybe this could be better:
>
> Return 0 if security attributes initialization is successful or not
> necessary, a negative value otherwise.

Well, I'm trying to avoid differentiating between the non-zero, but
successful attribute initialization and the zero attribute case; from
a caller's perspective it doesn't matter (and why we don't
differentiate between the two with different error codes).  If the
distinction between the two states is important from a caller's
perspective, there should be different return codes.

-- 
paul-moore.com
Re: [PATCH] security: Fix ret values doc for security_inode_init_security()
Posted by Roberto Sassu 2 years, 6 months ago 
On Tue, 2023-07-25 at 14:38 -0400, Paul Moore wrote:
> On Tue, Jul 25, 2023 at 3:02 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > On Mon, 2023-07-24 at 17:54 -0400, Paul Moore wrote:
> > > On Mon, Jul 24, 2023 at 10:52 AM Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > 
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > 
> > > > Commit 6bcdfd2cac55 ("security: Allow all LSMs to provide xattrs for
> > > > inode_init_security hook") unified the !initxattrs and initxattrs cases. By
> > > > doing that, security_inode_init_security() cannot return -EOPNOTSUPP
> > > > anymore, as it is always replaced with zero at the end of the function.
> > > > 
> > > > Also, mentioning -ENOMEM as the only possible error is not correct. For
> > > > example, evm_inode_init_security() could return -ENOKEY.
> > > > 
> > > > Fix these issues in the documentation of security_inode_init_security().
> > > > 
> > > > Fixes: 6bcdfd2cac55 ("security: Allow all LSMs to provide xattrs for inode_init_security hook")
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > ---
> > > >  security/security.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/security/security.c b/security/security.c
> > > > index cfdd0cbbcb9..5aa9cb91f0f 100644
> > > > --- a/security/security.c
> > > > +++ b/security/security.c
> > > > @@ -1604,8 +1604,8 @@ EXPORT_SYMBOL(security_dentry_create_files_as);
> > > >   * a security attribute on this particular inode, then it should return
> > > >   * -EOPNOTSUPP to skip this processing.
> > > >   *
> > > > - * Return: Returns 0 on success, -EOPNOTSUPP if no security attribute is
> > > > - * needed, or -ENOMEM on memory allocation failure.
> > > > + * Return: Returns 0 on success or on -EOPNOTSUPP error, a negative value other
> > > > + *         than -EOPNOTSUPP otherwise.
> > > 
> > > How about "Returns 0 if the LSM successfully initialized all of the
> > > inode security attributes that are required, negative values
> > > otherwise."?  The caller doesn't need to worry about the individual
> > > LSMs returning -EOPNOTSUPP in the case of no security attributes, and
> > > if they really care, they are likely reading the description above (or
> > > the code) which explains it in much better detail.
> > 
> > Maybe this could be better:
> > 
> > Return 0 if security attributes initialization is successful or not
> > necessary, a negative value otherwise.
> 
> Well, I'm trying to avoid differentiating between the non-zero, but
> successful attribute initialization and the zero attribute case; from
> a caller's perspective it doesn't matter (and why we don't
> differentiate between the two with different error codes).  If the
> distinction between the two states is important from a caller's
> perspective, there should be different return codes.

Ok, fine for me. I take your suggestion.

Thanks

Roberto

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.