Signed-off-by: Scott Matheina <scott(a)matheina.com&gt; --- kernel/auditfilter.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index 7714d93..774f9ad 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -39,13 +39,13 @@ * Locking model: * * audit_filter_mutex: - * Synchronizes writes and blocking reads of audit's filterlist - * data. Rcu is used to traverse the filterlist and access - * contents of structs audit_entry, audit_watch and opaque - * LSM rules during filtering. If modified, these structures - * must be copied and replace their counterparts in the filterlist. - * An audit_parent struct is not accessed during filtering, so may - * be written directly provided audit_filter_mutex is held. + * Synchronizes writes and blocking reads of audit's filterlist + * data. Rcu is used to traverse the filterlist and access + * contents of structs audit_entry, audit_watch and opaque + * LSM rules during filtering. If modified, these structures + * must be copied and replace their counterparts in the filterlist. + * An audit_parent struct is not accessed during filtering, so may + * be written directly provided audit_filter_mutex is held. */

/* Audit filter lists, defined in <linux/audit.h> */ @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head) { struct audit_entry *e = container_of(head, struct audit_entry, rcu); audit_free_rule(e); + }

/* Initialize an audit filterlist entry. */ @@ -176,9 +177,11 @@ static __u32 *classes[AUDIT_SYSCALL_CLASSES]; int __init audit_register_class(int class, unsigned *list) { __u32 *p = kcalloc(AUDIT_BITMASK_SIZE, sizeof(__u32), GFP_KERNEL); + if (!p) return -ENOMEM;

while (*list != ~0U) { + unsigned n = *list++; if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) { kfree(p);

On 10/14/2015 04:54 PM, Paul Moore wrote: > On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote: >> Signed-off-by: Scott Matheina <scott(a)matheina.com&gt; >> --- >> kernel/auditfilter.c | 17 ++++++++++------- >> 1 file changed, 10 insertions(+), 7 deletions(-) > Sorry for the delay in reviewing this, comments inline ... > >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c >> index 7714d93..774f9ad 100644 >> --- a/kernel/auditfilter.c >> +++ b/kernel/auditfilter.c >> @@ -39,13 +39,13 @@ >> * Locking model: >> * >> * audit_filter_mutex: >> - * Synchronizes writes and blocking reads of audit's filterlist >> - * data. Rcu is used to traverse the filterlist and access >> - * contents of structs audit_entry, audit_watch and opaque >> - * LSM rules during filtering. If modified, these structures >> - * must be copied and replace their counterparts in the filterlist. >> - * An audit_parent struct is not accessed during filtering, so may >> - * be written directly provided audit_filter_mutex is held. >> + * Synchronizes writes and blocking reads of audit's filterlist >> + * data. Rcu is used to traverse the filterlist and access >> + * contents of structs audit_entry, audit_watch and opaque >> + * LSM rules during filtering. If modified, these structures >> + * must be copied and replace their counterparts in the filterlist. >> + * An audit_parent struct is not accessed during filtering, so may >> + * be written directly provided audit_filter_mutex is held. >> */ > Okay, that's fine. > >> /* Audit filter lists, defined in <linux/audit.h> */ >> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head) >> { >> struct audit_entry *e = container_of(head, struct audit_entry, rcu); >> audit_free_rule(e); >> + >> } > Why? I was following the error messages in checkpatch.pl, but the warning went away after adding this line. No problem with the code.

>> /* Initialize an audit filterlist entry. */ >> @@ -176,9 +177,11 @@ static __u32 *classes[AUDIT_SYSCALL_CLASSES]; >> int __init audit_register_class(int class, unsigned *list) >> { >> __u32 *p = kcalloc(AUDIT_BITMASK_SIZE, sizeof(__u32), GFP_KERNEL); >> + >> if (!p) >> return -ENOMEM; > Okay. > >> while (*list != ~0U) { >> + >> unsigned n = *list++; >> if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) { >> kfree(p); > Why? This is the same as above. Just going through the checkpatch.pl script, and looking for warnings to fix.

As you might have guessed, this is one of my first patches. I wasn't sure if a patch like this would even get reviewed, and responded to. I'm subscribed to the linux-kernel mail group, and seeing what is acceptable. Thanks for the review. I don't plan on making a habit of submitting such incredibly trivial patches, but you have to start somewhere, and I thought it'd be hard to screw up by fixing a couple of trivial style errors.

On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote: > On 15/10/18, Scott Matheina wrote: > > On 10/14/2015 04:54 PM, Paul Moore wrote: > > > On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote: [] > > >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c [] > > >> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head) > > >> { > > >> struct audit_entry *e = container_of(head, struct audit_entry, rcu); > > >> audit_free_rule(e); > > >> + > > >> } > > > Why? > > > > I was following the error messages in checkpatch.pl, but the warning > > went away after adding this line. No problem with the code. > > That sounds like a bug in checkpatch.pl, since that blank line should be > tween the declaration and the function call. checkpatch message asks for a blank line after the "struct audit_entry *e = ..." declaration.

> > >> while (*list != ~0U) { > > >> + > > >> unsigned n = *list++; > > >> if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) { > > >> kfree(p); > > > Why? > > > > This is the same as above. Just going through the checkpatch.pl > > script, and looking for warnings to fix. > > Again, another manifestation of that bug? That blank line should be > after the declaration and before the if statement. [] > Well, I agree, you have to start somewhere... Too bad you hit a bug in > checkpatch.pl! Here too, not a bug in checkpatch. checkpatch output asks for a blank line after the "unsigned n" declaration, not before.

On 10/21/2015 10:33 AM, Richard Guy Briggs wrote: > On 15/10/21, Joe Perches wrote: >> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote: >>> On 15/10/18, Scott Matheina wrote: >>>> On 10/14/2015 04:54 PM, Paul Moore wrote: >>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote: >> [] >>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c >> [] >>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head) >>>>>> { >>>>>> struct audit_entry *e = container_of(head, struct audit_entry, rcu); >>>>>> audit_free_rule(e); >>>>>> + >>>>>> } >>>>> Why? >>>> I was following the error messages in checkpatch.pl, but the warning >>>> went away after adding this line. No problem with the code. >>> That sounds like a bug in checkpatch.pl, since that blank line should be >>> tween the declaration and the function call. >> checkpatch message asks for a blank line after the >> "struct audit_entry *e = ..." declaration. > Well then maybe it is a bug in his interpretation of the output of > checkpatch.pl? Scott, did you re-run checkpatch.pl after adding those > spaces? Did it pass? The error did go away.

>>>>>> while (*list != ~0U) { >>>>>> + >>>>>> unsigned n = *list++; >>>>>> if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) { >>>>>> kfree(p); >>>>> Why? >>>> This is the same as above. Just going through the checkpatch.pl >>>> script, and looking for warnings to fix. >>> Again, another manifestation of that bug? That blank line should be >>> after the declaration and before the if statement. >> [] >>> Well, I agree, you have to start somewhere... Too bad you hit a bug in >>> checkpatch.pl! >> Here too, not a bug in checkpatch. >> >> checkpatch output asks for a blank line after the >> "unsigned n" declaration, not before. > - RGB

On 10/21/2015 09:15 PM, Richard Guy Briggs wrote: > On 15/10/21, Scott Matheina wrote: >> On 10/21/2015 10:33 AM, Richard Guy Briggs wrote: >>> On 15/10/21, Joe Perches wrote: >>>> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote: >>>>> On 15/10/18, Scott Matheina wrote: >>>>>> On 10/14/2015 04:54 PM, Paul Moore wrote: >>>>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote: >>>> [] >>>>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c >>>> [] >>>>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head) >>>>>>>> { >>>>>>>> struct audit_entry *e = container_of(head, struct audit_entry, rcu); >>>>>>>> audit_free_rule(e); >>>>>>>> + >>>>>>>> } >>>>>>> Why? >>>>>> I was following the error messages in checkpatch.pl, but the warning >>>>>> went away after adding this line. No problem with the code. >>>>> That sounds like a bug in checkpatch.pl, since that blank line should be >>>>> tween the declaration and the function call. >>>> checkpatch message asks for a blank line after the >>>> "struct audit_entry *e = ..." declaration. >>> Well then maybe it is a bug in his interpretation of the output of >>> checkpatch.pl? Scott, did you re-run checkpatch.pl after adding those >>> spaces? Did it pass? >> The error did go away. > Joe, I confirm the error went away. Looks like a bug in checkpatch.pl > to me. I tried a number of combinations of things and it didn't > complain about several things it should have. I did try a few other > things to make sure it was still finding problems like brace placement > and leading spaces, but it looks like the blank line checking code isn't > working. This is on 4.0, so maybe it has been fixed since then. Scott, > what kernel version are you using? I'm running Ubuntu 14.04 LTS (Kernel 3.19.0-30-generic) on my machine. I cloned Linus' repo for source code. I'm pretty sure you were talking about the active Kernel on my machine, so if not please let me know.

>>>>>>>> while (*list != ~0U) { >>>>>>>> + >>>>>>>> unsigned n = *list++; >>>>>>>> if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) { >>>>>>>> kfree(p); >>>>>>> Why? >>>>>> This is the same as above. Just going through the checkpatch.pl >>>>>> script, and looking for warnings to fix. >>>>> Again, another manifestation of that bug? That blank line should be >>>>> after the declaration and before the if statement. >>>> [] >>>>> Well, I agree, you have to start somewhere... Too bad you hit a bug in >>>>> checkpatch.pl! >>>> Here too, not a bug in checkpatch. >>>> >>>> checkpatch output asks for a blank line after the >>>> "unsigned n" declaration, not before. >>> - RGB > - RGB

On 15/10/21, Scott Matheina wrote: > On 10/21/2015 10:33 AM, Richard Guy Briggs wrote: > > On 15/10/21, Joe Perches wrote: > >> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote: > >>> On 15/10/18, Scott Matheina wrote: > >>>> On 10/14/2015 04:54 PM, Paul Moore wrote: > >>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote: > >> [] > >>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > >> [] > >>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head) > >>>>>> { > >>>>>> struct audit_entry *e = container_of(head, struct audit_entry, rcu); > >>>>>> audit_free_rule(e); > >>>>>> + > >>>>>> } > >>>>> Why? > >>>> I was following the error messages in checkpatch.pl, but the warning > >>>> went away after adding this line. No problem with the code. > >>> That sounds like a bug in checkpatch.pl, since that blank line should be > >>> tween the declaration and the function call. > >> checkpatch message asks for a blank line after the > >> "struct audit_entry *e = ..." declaration. > > Well then maybe it is a bug in his interpretation of the output of > > checkpatch.pl? Scott, did you re-run checkpatch.pl after adding those > > spaces? Did it pass? > > The error did go away. Joe, I confirm the error went away. Looks like a bug in checkpatch.pl to me.

On Sunday, October 18, 2015 12:50:45 PM Scott Matheina wrote: > On 10/14/2015 04:54 PM, Paul Moore wrote: >> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote: >>> Signed-off-by: Scott Matheina <scott(a)matheina.com&gt; >>> --- >>> >>> kernel/auditfilter.c | 17 ++++++++++------- >>> 1 file changed, 10 insertions(+), 7 deletions(-) >> Sorry for the delay in reviewing this, comments inline ... >> >>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c >>> index 7714d93..774f9ad 100644 >>> --- a/kernel/auditfilter.c >>> +++ b/kernel/auditfilter.c >>> @@ -39,13 +39,13 @@ >>> >>> * Locking model: >>> * >>> >>> * audit_filter_mutex: >>> - * Synchronizes writes and blocking reads of audit's filterlist >>> - * data. Rcu is used to traverse the filterlist and access >>> - * contents of structs audit_entry, audit_watch and opaque >>> - * LSM rules during filtering. If modified, these structures >>> - * must be copied and replace their counterparts in the filterlist. >>> - * An audit_parent struct is not accessed during filtering, so may >>> - * be written directly provided audit_filter_mutex is held. >>> + * Synchronizes writes and blocking reads of audit's filterlist >>> + * data. Rcu is used to traverse the filterlist and access >>> + * contents of structs audit_entry, audit_watch and opaque >>> + * LSM rules during filtering. If modified, these structures >>> + * must be copied and replace their counterparts in the filterlist. >>> + * An audit_parent struct is not accessed during filtering, so may >>> + * be written directly provided audit_filter_mutex is held. >>> >>> */ >> Okay, that's fine. ... > As you might have guessed, this is one of my first patches. I wasn't sure if > a patch like this would even get reviewed, and responded to. I'm subscribed > to the linux-kernel mail group, and seeing what is acceptable. > > Thanks for the review. I don't plan on making a habit of submitting such > incredibly trivial patches, but you have to start somewhere, and I thought > it'd be hard to screw up by fixing a couple of trivial style errors. We all start somewhere, and with that in mind, if you want to resubmit this patch with only the fix above (fixing the whitespace in the comment block), I'll apply it. While the patch is trivial, it is does fix a minor nit with near-zero risk. I would encourage you to try something a bit more substantial next time, as they say, bug fixes are the quickest way to a maintainer's heart.