| Message ID | 20230217032625.678457-1-paul@paul-moore.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Move LSM hook comments into security/security.c | expand |
On Thu, Feb 16, 2023 at 10:26 PM Paul Moore <paul@paul-moore.com> wrote: > > Hello all, > > The LSM hook comment blocks are a in a rather sad state; separated from > the hook definitions they are often out of mind, and as a result > most of them are in varying levels of bit-rot, some severely. This > patchset moves all of the comment blocks out of lsm_hooks.c and onto > the top of the function definitions as one would normally expect. ... > include/linux/lsm_hooks.h | 1624 +++++++++++++++++++++ > security/security.c | 2702 +--------------------------------------- > 2 files changed, 1710 insertions(+), 2616 deletions(-) I just realized that in my excitement of finally finishing this tedious task I accidentally ran the diff backwards so the diffstat looks a little odd - sorry about that - the correct diffstat is below: include/linux/lsm_hooks.h | 1624 --------------------- security/security.c | 2708 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 2619 insertions(+), 1713 deletions(-)
On 2/16/2023 7:26 PM, Paul Moore wrote: > Hello all, > > The LSM hook comment blocks are a in a rather sad state; separated from > the hook definitions they are often out of mind, and as a result > most of them are in varying levels of bit-rot, some severely. This > patchset moves all of the comment blocks out of lsm_hooks.c and onto > the top of the function definitions as one would normally expect. > In the process of moving the comment blocks, they have been massaged > into the standard kernel-doc format for the sake of consistency and > easier reading. Unfortunately, correcting all of the errors in the > comments would have made an extremely long and painful task even worse, > so a number of errors remain, but the worst offenders were corrected in > the move. Now that the comments are in the proper location, and in the > proper format, my hope is that future patch submissions correcting the > actual comment contents will be much easier and the comments as a whole > will be easier to maintain. > > There are no code changes in this patchset, although since I was > already adding a lot of churn to security.c, the last patch in this > patchset (22/22) does take the liberty of fixing some rather ugly > style problems. > > include/linux/lsm_hooks.h | 1624 +++++++++++++++++++++ > security/security.c | 2702 +--------------------------------------- > 2 files changed, 1710 insertions(+), 2616 deletions(-) Except for the reversed diffstat (which you point out elsewhere) this looks fine. There's a lot of work to be done to make the comment content up to snuff, but that's another barrel of kittens. For the set: Acked-by: Casey Schaufler <casey@schaufler-ca.com>
On Fri, Feb 17, 2023 at 12:22 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > ... There's a lot of work to be done to make the comment > content up to snuff, but that's another barrel of kittens. Yes, most definitely. My hope is that this patchset gives us a better chance at both noticing that the comments are out-of-sync/bad and maintaining them long term. I'm also hoping that other folks will feel free to submit patches correcting the comment blocks for their favorite LSM hooks after this lands in lsm/next :)
On Thu, Feb 16, 2023 at 10:26 PM Paul Moore <paul@paul-moore.com> wrote: > > Hello all, > > The LSM hook comment blocks are a in a rather sad state; separated from > the hook definitions they are often out of mind, and as a result > most of them are in varying levels of bit-rot, some severely. This > patchset moves all of the comment blocks out of lsm_hooks.c and onto > the top of the function definitions as one would normally expect. > In the process of moving the comment blocks, they have been massaged > into the standard kernel-doc format for the sake of consistency and > easier reading. Unfortunately, correcting all of the errors in the > comments would have made an extremely long and painful task even worse, > so a number of errors remain, but the worst offenders were corrected in > the move. Now that the comments are in the proper location, and in the > proper format, my hope is that future patch submissions correcting the > actual comment contents will be much easier and the comments as a whole > will be easier to maintain. > > There are no code changes in this patchset, although since I was > already adding a lot of churn to security.c, the last patch in this > patchset (22/22) does take the liberty of fixing some rather ugly > style problems. > > include/linux/lsm_hooks.h | 1624 +++++++++++++++++++++ > security/security.c | 2702 +--------------------------------------- > 2 files changed, 1710 insertions(+), 2616 deletions(-) Seeing no objections, and the ACK from Casey, I've gone ahead and merged this patchset into the lsm/next branch. There was some minor merge fuzz due to the mount idmap work and some IMA changes, but the vast majority of the patchset is exactly as posted.
On Mon, 2023-03-06 at 13:49 -0500, Paul Moore wrote: > On Thu, Feb 16, 2023 at 10:26 PM Paul Moore <paul@paul-moore.com> wrote: > > Hello all, > > > > The LSM hook comment blocks are a in a rather sad state; separated from > > the hook definitions they are often out of mind, and as a result > > most of them are in varying levels of bit-rot, some severely. This > > patchset moves all of the comment blocks out of lsm_hooks.c and onto > > the top of the function definitions as one would normally expect. > > In the process of moving the comment blocks, they have been massaged > > into the standard kernel-doc format for the sake of consistency and > > easier reading. Unfortunately, correcting all of the errors in the > > comments would have made an extremely long and painful task even worse, > > so a number of errors remain, but the worst offenders were corrected in > > the move. Now that the comments are in the proper location, and in the > > proper format, my hope is that future patch submissions correcting the > > actual comment contents will be much easier and the comments as a whole > > will be easier to maintain. > > > > There are no code changes in this patchset, although since I was > > already adding a lot of churn to security.c, the last patch in this > > patchset (22/22) does take the liberty of fixing some rather ugly > > style problems. > > > > include/linux/lsm_hooks.h | 1624 +++++++++++++++++++++ > > security/security.c | 2702 +--------------------------------------- > > 2 files changed, 1710 insertions(+), 2616 deletions(-) > > Seeing no objections, and the ACK from Casey, I've gone ahead and > merged this patchset into the lsm/next branch. There was some minor > merge fuzz due to the mount idmap work and some IMA changes, but the > vast majority of the patchset is exactly as posted. Oh, I thought it was an intermediate version and didn't report some issues: scripts/kernel-doc security/security.c|grep warning security/security.c:1236: warning: Function parameter or member 'mnt_opts' not described in 'security_free_mnt_opts' security/security.c:1236: warning: Excess function parameter 'mnt_ops' description in 'security_free_mnt_opts' security/security.c:1254: warning: Function parameter or member 'mnt_opts' not described in 'security_sb_eat_lsm_opts' security/security.c:1254: warning: Excess function parameter 'mnt_ops' description in 'security_sb_eat_lsm_opts' security/security.c:1423: warning: Function parameter or member 'oldsb' not described in 'security_sb_clone_mnt_opts' security/security.c:1423: warning: Function parameter or member 'newsb' not described in 'security_sb_clone_mnt_opts' [...] Roberto
On Tue, Mar 7, 2023 at 3:09 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > On Mon, 2023-03-06 at 13:49 -0500, Paul Moore wrote: > > On Thu, Feb 16, 2023 at 10:26 PM Paul Moore <paul@paul-moore.com> wrote: > > > Hello all, > > > > > > The LSM hook comment blocks are a in a rather sad state; separated from > > > the hook definitions they are often out of mind, and as a result > > > most of them are in varying levels of bit-rot, some severely. This > > > patchset moves all of the comment blocks out of lsm_hooks.c and onto > > > the top of the function definitions as one would normally expect. > > > In the process of moving the comment blocks, they have been massaged > > > into the standard kernel-doc format for the sake of consistency and > > > easier reading. Unfortunately, correcting all of the errors in the > > > comments would have made an extremely long and painful task even worse, > > > so a number of errors remain, but the worst offenders were corrected in > > > the move. Now that the comments are in the proper location, and in the > > > proper format, my hope is that future patch submissions correcting the > > > actual comment contents will be much easier and the comments as a whole > > > will be easier to maintain. > > > > > > There are no code changes in this patchset, although since I was > > > already adding a lot of churn to security.c, the last patch in this > > > patchset (22/22) does take the liberty of fixing some rather ugly > > > style problems. > > > > > > include/linux/lsm_hooks.h | 1624 +++++++++++++++++++++ > > > security/security.c | 2702 +--------------------------------------- > > > 2 files changed, 1710 insertions(+), 2616 deletions(-) > > > > Seeing no objections, and the ACK from Casey, I've gone ahead and > > merged this patchset into the lsm/next branch. There was some minor > > merge fuzz due to the mount idmap work and some IMA changes, but the > > vast majority of the patchset is exactly as posted. > > Oh, I thought it was an intermediate version and didn't report some > issues: If you don't see a "RFC" in the patch subject line it's safe to assume it is a "final" version. Regardless, feedback is never bad, even if it is a RFC. > scripts/kernel-doc security/security.c|grep warning > security/security.c:1236: warning: Function parameter or member 'mnt_opts' not described in 'security_free_mnt_opts' > security/security.c:1236: warning: Excess function parameter 'mnt_ops' description in 'security_free_mnt_opts' > security/security.c:1254: warning: Function parameter or member 'mnt_opts' not described in 'security_sb_eat_lsm_opts' > security/security.c:1254: warning: Excess function parameter 'mnt_ops' description in 'security_sb_eat_lsm_opts' > security/security.c:1423: warning: Function parameter or member 'oldsb' not described in 'security_sb_clone_mnt_opts' > security/security.c:1423: warning: Function parameter or member 'newsb' not described in 'security_sb_clone_mnt_opts' Unsurprising. Those patches were mostly just to relocate the comment blocks out of lsm_hooks.h and into security.c; while I did fix some of the really bad errors, fixing everything in the move wasn't really the goal, that's for future work. Did you want to submit a patch to fix those?
On Tue, 2023-03-07 at 11:33 -0500, Paul Moore wrote: > On Tue, Mar 7, 2023 at 3:09 AM Roberto Sassu > <roberto.sassu@huaweicloud.com> wrote: > > On Mon, 2023-03-06 at 13:49 -0500, Paul Moore wrote: > > > On Thu, Feb 16, 2023 at 10:26 PM Paul Moore <paul@paul-moore.com> wrote: > > > > Hello all, > > > > > > > > The LSM hook comment blocks are a in a rather sad state; separated from > > > > the hook definitions they are often out of mind, and as a result > > > > most of them are in varying levels of bit-rot, some severely. This > > > > patchset moves all of the comment blocks out of lsm_hooks.c and onto > > > > the top of the function definitions as one would normally expect. > > > > In the process of moving the comment blocks, they have been massaged > > > > into the standard kernel-doc format for the sake of consistency and > > > > easier reading. Unfortunately, correcting all of the errors in the > > > > comments would have made an extremely long and painful task even worse, > > > > so a number of errors remain, but the worst offenders were corrected in > > > > the move. Now that the comments are in the proper location, and in the > > > > proper format, my hope is that future patch submissions correcting the > > > > actual comment contents will be much easier and the comments as a whole > > > > will be easier to maintain. > > > > > > > > There are no code changes in this patchset, although since I was > > > > already adding a lot of churn to security.c, the last patch in this > > > > patchset (22/22) does take the liberty of fixing some rather ugly > > > > style problems. > > > > > > > > include/linux/lsm_hooks.h | 1624 +++++++++++++++++++++ > > > > security/security.c | 2702 +--------------------------------------- > > > > 2 files changed, 1710 insertions(+), 2616 deletions(-) > > > > > > Seeing no objections, and the ACK from Casey, I've gone ahead and > > > merged this patchset into the lsm/next branch. There was some minor > > > merge fuzz due to the mount idmap work and some IMA changes, but the > > > vast majority of the patchset is exactly as posted. > > > > Oh, I thought it was an intermediate version and didn't report some > > issues: > > If you don't see a "RFC" in the patch subject line it's safe to assume > it is a "final" version. Regardless, feedback is never bad, even if > it is a RFC. > > > scripts/kernel-doc security/security.c|grep warning > > security/security.c:1236: warning: Function parameter or member 'mnt_opts' not described in 'security_free_mnt_opts' > > security/security.c:1236: warning: Excess function parameter 'mnt_ops' description in 'security_free_mnt_opts' > > security/security.c:1254: warning: Function parameter or member 'mnt_opts' not described in 'security_sb_eat_lsm_opts' > > security/security.c:1254: warning: Excess function parameter 'mnt_ops' description in 'security_sb_eat_lsm_opts' > > security/security.c:1423: warning: Function parameter or member 'oldsb' not described in 'security_sb_clone_mnt_opts' > > security/security.c:1423: warning: Function parameter or member 'newsb' not described in 'security_sb_clone_mnt_opts' > > Unsurprising. Those patches were mostly just to relocate the comment > blocks out of lsm_hooks.h and into security.c; while I did fix some of > the really bad errors, fixing everything in the move wasn't really the > goal, that's for future work. > > Did you want to submit a patch to fix those? I rebased the stacked IMA/EVM to your patch set, so that it is closer to the final version. I expect there will not be too many conflicts. It is also ok for me to fix those issues in the future. Roberto
On Tue, Mar 7, 2023 at 11:38 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > On Tue, 2023-03-07 at 11:33 -0500, Paul Moore wrote: > > On Tue, Mar 7, 2023 at 3:09 AM Roberto Sassu > > <roberto.sassu@huaweicloud.com> wrote: > > > On Mon, 2023-03-06 at 13:49 -0500, Paul Moore wrote: > > > > On Thu, Feb 16, 2023 at 10:26 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > Hello all, > > > > > > > > > > The LSM hook comment blocks are a in a rather sad state; separated from > > > > > the hook definitions they are often out of mind, and as a result > > > > > most of them are in varying levels of bit-rot, some severely. This > > > > > patchset moves all of the comment blocks out of lsm_hooks.c and onto > > > > > the top of the function definitions as one would normally expect. > > > > > In the process of moving the comment blocks, they have been massaged > > > > > into the standard kernel-doc format for the sake of consistency and > > > > > easier reading. Unfortunately, correcting all of the errors in the > > > > > comments would have made an extremely long and painful task even worse, > > > > > so a number of errors remain, but the worst offenders were corrected in > > > > > the move. Now that the comments are in the proper location, and in the > > > > > proper format, my hope is that future patch submissions correcting the > > > > > actual comment contents will be much easier and the comments as a whole > > > > > will be easier to maintain. > > > > > > > > > > There are no code changes in this patchset, although since I was > > > > > already adding a lot of churn to security.c, the last patch in this > > > > > patchset (22/22) does take the liberty of fixing some rather ugly > > > > > style problems. > > > > > > > > > > include/linux/lsm_hooks.h | 1624 +++++++++++++++++++++ > > > > > security/security.c | 2702 +--------------------------------------- > > > > > 2 files changed, 1710 insertions(+), 2616 deletions(-) > > > > > > > > Seeing no objections, and the ACK from Casey, I've gone ahead and > > > > merged this patchset into the lsm/next branch. There was some minor > > > > merge fuzz due to the mount idmap work and some IMA changes, but the > > > > vast majority of the patchset is exactly as posted. > > > > > > Oh, I thought it was an intermediate version and didn't report some > > > issues: > > > > If you don't see a "RFC" in the patch subject line it's safe to assume > > it is a "final" version. Regardless, feedback is never bad, even if > > it is a RFC. > > > > > scripts/kernel-doc security/security.c|grep warning > > > security/security.c:1236: warning: Function parameter or member 'mnt_opts' not described in 'security_free_mnt_opts' > > > security/security.c:1236: warning: Excess function parameter 'mnt_ops' description in 'security_free_mnt_opts' > > > security/security.c:1254: warning: Function parameter or member 'mnt_opts' not described in 'security_sb_eat_lsm_opts' > > > security/security.c:1254: warning: Excess function parameter 'mnt_ops' description in 'security_sb_eat_lsm_opts' > > > security/security.c:1423: warning: Function parameter or member 'oldsb' not described in 'security_sb_clone_mnt_opts' > > > security/security.c:1423: warning: Function parameter or member 'newsb' not described in 'security_sb_clone_mnt_opts' > > > > Unsurprising. Those patches were mostly just to relocate the comment > > blocks out of lsm_hooks.h and into security.c; while I did fix some of > > the really bad errors, fixing everything in the move wasn't really the > > goal, that's for future work. > > > > Did you want to submit a patch to fix those? > > I rebased the stacked IMA/EVM to your patch set, so that it is closer > to the final version. I expect there will not be too many conflicts. > > It is also ok for me to fix those issues in the future. That would be great, thanks.
On Wed, 2023-03-08 at 12:09 -0500, Paul Moore wrote: > On Tue, Mar 7, 2023 at 11:38 AM Roberto Sassu > <roberto.sassu@huaweicloud.com> wrote: > > On Tue, 2023-03-07 at 11:33 -0500, Paul Moore wrote: > > > On Tue, Mar 7, 2023 at 3:09 AM Roberto Sassu > > > <roberto.sassu@huaweicloud.com> wrote: > > > > On Mon, 2023-03-06 at 13:49 -0500, Paul Moore wrote: > > > > > On Thu, Feb 16, 2023 at 10:26 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > > Hello all, > > > > > > > > > > > > The LSM hook comment blocks are a in a rather sad state; separated from > > > > > > the hook definitions they are often out of mind, and as a result > > > > > > most of them are in varying levels of bit-rot, some severely. This > > > > > > patchset moves all of the comment blocks out of lsm_hooks.c and onto > > > > > > the top of the function definitions as one would normally expect. > > > > > > In the process of moving the comment blocks, they have been massaged > > > > > > into the standard kernel-doc format for the sake of consistency and > > > > > > easier reading. Unfortunately, correcting all of the errors in the > > > > > > comments would have made an extremely long and painful task even worse, > > > > > > so a number of errors remain, but the worst offenders were corrected in > > > > > > the move. Now that the comments are in the proper location, and in the > > > > > > proper format, my hope is that future patch submissions correcting the > > > > > > actual comment contents will be much easier and the comments as a whole > > > > > > will be easier to maintain. > > > > > > > > > > > > There are no code changes in this patchset, although since I was > > > > > > already adding a lot of churn to security.c, the last patch in this > > > > > > patchset (22/22) does take the liberty of fixing some rather ugly > > > > > > style problems. > > > > > > > > > > > > include/linux/lsm_hooks.h | 1624 +++++++++++++++++++++ > > > > > > security/security.c | 2702 +--------------------------------------- > > > > > > 2 files changed, 1710 insertions(+), 2616 deletions(-) > > > > > > > > > > Seeing no objections, and the ACK from Casey, I've gone ahead and > > > > > merged this patchset into the lsm/next branch. There was some minor > > > > > merge fuzz due to the mount idmap work and some IMA changes, but the > > > > > vast majority of the patchset is exactly as posted. > > > > > > > > Oh, I thought it was an intermediate version and didn't report some > > > > issues: > > > > > > If you don't see a "RFC" in the patch subject line it's safe to assume > > > it is a "final" version. Regardless, feedback is never bad, even if > > > it is a RFC. > > > > > > > scripts/kernel-doc security/security.c|grep warning > > > > security/security.c:1236: warning: Function parameter or member 'mnt_opts' not described in 'security_free_mnt_opts' > > > > security/security.c:1236: warning: Excess function parameter 'mnt_ops' description in 'security_free_mnt_opts' > > > > security/security.c:1254: warning: Function parameter or member 'mnt_opts' not described in 'security_sb_eat_lsm_opts' > > > > security/security.c:1254: warning: Excess function parameter 'mnt_ops' description in 'security_sb_eat_lsm_opts' > > > > security/security.c:1423: warning: Function parameter or member 'oldsb' not described in 'security_sb_clone_mnt_opts' > > > > security/security.c:1423: warning: Function parameter or member 'newsb' not described in 'security_sb_clone_mnt_opts' > > > > > > Unsurprising. Those patches were mostly just to relocate the comment > > > blocks out of lsm_hooks.h and into security.c; while I did fix some of > > > the really bad errors, fixing everything in the move wasn't really the > > > goal, that's for future work. > > > > > > Did you want to submit a patch to fix those? > > > > I rebased the stacked IMA/EVM to your patch set, so that it is closer > > to the final version. I expect there will not be too many conflicts. > > > > It is also ok for me to fix those issues in the future. > > That would be great, thanks. I meant generically someone... (ok, I got the task). Roberto
On Wed, Mar 8, 2023 at 12:14 PM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > > On Wed, 2023-03-08 at 12:09 -0500, Paul Moore wrote: > > On Tue, Mar 7, 2023 at 11:38 AM Roberto Sassu > > <roberto.sassu@huaweicloud.com> wrote: > > > On Tue, 2023-03-07 at 11:33 -0500, Paul Moore wrote: > > > > On Tue, Mar 7, 2023 at 3:09 AM Roberto Sassu > > > > <roberto.sassu@huaweicloud.com> wrote: > > > > > On Mon, 2023-03-06 at 13:49 -0500, Paul Moore wrote: > > > > > > On Thu, Feb 16, 2023 at 10:26 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > > > Hello all, > > > > > > > > > > > > > > The LSM hook comment blocks are a in a rather sad state; separated from > > > > > > > the hook definitions they are often out of mind, and as a result > > > > > > > most of them are in varying levels of bit-rot, some severely. This > > > > > > > patchset moves all of the comment blocks out of lsm_hooks.c and onto > > > > > > > the top of the function definitions as one would normally expect. > > > > > > > In the process of moving the comment blocks, they have been massaged > > > > > > > into the standard kernel-doc format for the sake of consistency and > > > > > > > easier reading. Unfortunately, correcting all of the errors in the > > > > > > > comments would have made an extremely long and painful task even worse, > > > > > > > so a number of errors remain, but the worst offenders were corrected in > > > > > > > the move. Now that the comments are in the proper location, and in the > > > > > > > proper format, my hope is that future patch submissions correcting the > > > > > > > actual comment contents will be much easier and the comments as a whole > > > > > > > will be easier to maintain. > > > > > > > > > > > > > > There are no code changes in this patchset, although since I was > > > > > > > already adding a lot of churn to security.c, the last patch in this > > > > > > > patchset (22/22) does take the liberty of fixing some rather ugly > > > > > > > style problems. > > > > > > > > > > > > > > include/linux/lsm_hooks.h | 1624 +++++++++++++++++++++ > > > > > > > security/security.c | 2702 +--------------------------------------- > > > > > > > 2 files changed, 1710 insertions(+), 2616 deletions(-) > > > > > > > > > > > > Seeing no objections, and the ACK from Casey, I've gone ahead and > > > > > > merged this patchset into the lsm/next branch. There was some minor > > > > > > merge fuzz due to the mount idmap work and some IMA changes, but the > > > > > > vast majority of the patchset is exactly as posted. > > > > > > > > > > Oh, I thought it was an intermediate version and didn't report some > > > > > issues: > > > > > > > > If you don't see a "RFC" in the patch subject line it's safe to assume > > > > it is a "final" version. Regardless, feedback is never bad, even if > > > > it is a RFC. > > > > > > > > > scripts/kernel-doc security/security.c|grep warning > > > > > security/security.c:1236: warning: Function parameter or member 'mnt_opts' not described in 'security_free_mnt_opts' > > > > > security/security.c:1236: warning: Excess function parameter 'mnt_ops' description in 'security_free_mnt_opts' > > > > > security/security.c:1254: warning: Function parameter or member 'mnt_opts' not described in 'security_sb_eat_lsm_opts' > > > > > security/security.c:1254: warning: Excess function parameter 'mnt_ops' description in 'security_sb_eat_lsm_opts' > > > > > security/security.c:1423: warning: Function parameter or member 'oldsb' not described in 'security_sb_clone_mnt_opts' > > > > > security/security.c:1423: warning: Function parameter or member 'newsb' not described in 'security_sb_clone_mnt_opts' > > > > > > > > Unsurprising. Those patches were mostly just to relocate the comment > > > > blocks out of lsm_hooks.h and into security.c; while I did fix some of > > > > the really bad errors, fixing everything in the move wasn't really the > > > > goal, that's for future work. > > > > > > > > Did you want to submit a patch to fix those? > > > > > > I rebased the stacked IMA/EVM to your patch set, so that it is closer > > > to the final version. I expect there will not be too many conflicts. > > > > > > It is also ok for me to fix those issues in the future. > > > > That would be great, thanks. > > I meant generically someone... (ok, I got the task). Ah, okay, no worries. I'll go ahead and fixup the current kdoc warnings (today?), but the comment blocks could still use a bit of review to fix/improve the quality of the comments in general.