[PATCH 0/7] security: apparmor: apparmorfs.c Checkpatch mods

[PATCH 0/7] security: apparmor: apparmorfs.c Checkpatch mods

[Walt Feasel]

Make checkpatch modifications to include:

Multiple blank lines
Blank line after declarations
Space after cast
WARN_ON vs BUG_ON
Align parenthesis
Comparison to NULL
Line continuations

Walt Feasel (7):
  security: apparmor: apparmorfs.c Multiple blank lines
  security: apparmor: apparmorfs.c Blank line after declarations
  security: apparmor: apparmorfs.c Space after cast
  security: apparmor: apparmorfs.c WARN_ON vs BUG_ON
  security: apparmor: apparmorfs.c Align parenthesis
  security: apparmor: apparmorfs.c Comparison to NULL
  security: apparmor: apparmorfs.c Line continuations

 security/apparmor/apparmorfs.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

--
Have questions on if the WARN_ON is used properly in patch 4
and also for the comparrison to NULL patch in patch 6

2.1.4

[PATCH 1/7] security: apparmor: apparmorfs.c Multiple blank lines

[Walt Feasel]

Make style modifications for:
CHECK: Please don't use multiple blank lines

Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
---
 security/apparmor/apparmorfs.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 5923d56..62aac35 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -112,7 +112,6 @@ static char *aa_simple_write_to_buffer(int op, const char __user *userbuf,
 	return data;
 }
 
-
 /* .load file hook fn to load policy */
 static ssize_t profile_load(struct file *f, const char __user *buf, size_t size,
 			    loff_t *pos)
@@ -552,7 +551,6 @@ int __aa_fs_namespace_mkdir(struct aa_namespace *ns, struct dentry *parent,
 	return error;
 }
 
-
 #define list_entry_is_head(pos, head, member) (&pos->member == (head))
 
 /**
@@ -687,7 +685,6 @@ static void *p_start(struct seq_file *f, loff_t *pos)
 	loff_t l = *pos;
 	f->private = aa_get_namespace(root);
 
-
 	/* find the first profile */
 	mutex_lock(&root->lock);
 	profile = __first_profile(root, root);
@@ -782,7 +779,6 @@ static const struct file_operations aa_fs_profiles_fops = {
 	.release = profiles_release,
 };
 
-
 /** Base file system setup **/
 static struct aa_fs_entry aa_fs_entry_file[] = {
 	AA_FS_FILE_STRING("mask", "create read write exec append mmap_exec " \
-- 
2.1.4

[PATCH 2/7] security: apparmor: apparmorfs.c Blank line after declarations

[Walt Feasel]

Make style modifications for:
WARNING: Missing a blank line after declarations

Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
---
 security/apparmor/apparmorfs.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 62aac35..6b9ace3 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -57,6 +57,7 @@ static int mangle_name(char *name, char *target)
 		*t = 0;
 	} else {
 		int len = 0;
+
 		for (; *name; name++) {
 			if (isalnum(*name) || isspace(*name) ||
 			    strchr("/._-", *name))
@@ -240,6 +241,7 @@ static int aa_fs_seq_profile_open(struct inode *inode, struct file *file,
 static int aa_fs_seq_profile_release(struct inode *inode, struct file *file)
 {
 	struct seq_file *seq = (struct seq_file *) file->private_data;
+
 	if (seq)
 		aa_put_replacedby(seq->private);
 	return single_release(inode, file);
@@ -249,6 +251,7 @@ static int aa_fs_seq_profname_show(struct seq_file *seq, void *v)
 {
 	struct aa_replacedby *r = seq->private;
 	struct aa_profile *profile = aa_get_profile_rcu(&r->profile);
+
 	seq_printf(seq, "%s\n", profile->base.name);
 	aa_put_profile(profile);
 
@@ -272,6 +275,7 @@ static int aa_fs_seq_profmode_show(struct seq_file *seq, void *v)
 {
 	struct aa_replacedby *r = seq->private;
 	struct aa_profile *profile = aa_get_profile_rcu(&r->profile);
+
 	seq_printf(seq, "%s\n", aa_profile_mode_names[profile->mode]);
 	aa_put_profile(profile);
 
@@ -295,6 +299,7 @@ static int aa_fs_seq_profattach_show(struct seq_file *seq, void *v)
 {
 	struct aa_replacedby *r = seq->private;
 	struct aa_profile *profile = aa_get_profile_rcu(&r->profile);
+
 	if (profile->attach)
 		seq_printf(seq, "%s\n", profile->attach);
 	else if (profile->xmatch)
@@ -362,6 +367,7 @@ void __aa_fs_profile_rmdir(struct aa_profile *profile)
 
 	for (i = AAFS_PROF_SIZEOF - 1; i >= 0; --i) {
 		struct aa_replacedby *r;
+
 		if (!profile->dents[i])
 			continue;
 
@@ -408,6 +414,7 @@ int __aa_fs_profile_mkdir(struct aa_profile *profile, struct dentry *parent)
 
 	if (!parent) {
 		struct aa_profile *p;
+
 		p = aa_deref_parent(profile);
 		dent = prof_dir(p);
 		/* adding to parent that previously didn't have children */
@@ -419,6 +426,7 @@ int __aa_fs_profile_mkdir(struct aa_profile *profile, struct dentry *parent)
 
 	if (!profile->dirname) {
 		int len, id_len;
+
 		len = mangle_name(profile->base.name, NULL);
 		id_len = snprintf(NULL, 0, ".%ld", profile->ns->uniq_id);
 
@@ -662,6 +670,7 @@ static struct aa_profile *next_profile(struct aa_namespace *root,
 				       struct aa_profile *profile)
 {
 	struct aa_profile *next = __next_profile(profile);
+
 	if (next)
 		return next;
 
@@ -683,6 +692,7 @@ static void *p_start(struct seq_file *f, loff_t *pos)
 	struct aa_profile *profile = NULL;
 	struct aa_namespace *root = aa_current_profile()->ns;
 	loff_t l = *pos;
+
 	f->private = aa_get_namespace(root);
 
 	/* find the first profile */
-- 
2.1.4

[PATCH 3/7] security: apparmor: apparmorfs.c Space after cast

[Walt Feasel]

Make style modifications for:
CHECK: No space is necessary after a cast

Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
---
 security/apparmor/apparmorfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 6b9ace3..c5701f3 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -240,7 +240,7 @@ static int aa_fs_seq_profile_open(struct inode *inode, struct file *file,
 
 static int aa_fs_seq_profile_release(struct inode *inode, struct file *file)
 {
-	struct seq_file *seq = (struct seq_file *) file->private_data;
+	struct seq_file *seq = (struct seq_file *)file->private_data;
 
 	if (seq)
 		aa_put_replacedby(seq->private);
-- 
2.1.4

[PATCH 4/7] security: apparmor: apparmorfs.c WARN_ON vs BUG_ON

[Walt Feasel]

Make style modifications for:
CHECK: Avoid crashing the kernel - try using WARN_ON
& recovery code rather than BUG() or BUG_ON()

Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
---
Not sure if correct use. Seems if copy_size > alloc_size
it is corrected by writing partial data and return an error.

 security/apparmor/apparmorfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index c5701f3..d759c78 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -87,7 +87,7 @@ static char *aa_simple_write_to_buffer(int op, const char __user *userbuf,
 {
 	char *data;
 
-	BUG_ON(copy_size > alloc_size);
+	WARN_ON(copy_size > alloc_size);
 
 	if (*pos != 0)
 		/* only writes from pos 0, that is complete writes */
-- 
2.1.4

[PATCH 5/7] security: apparmor: apparmorfs.c Align parenthesis

[Walt Feasel]

Make style modifications for:
CHECK: Alignment should match open parenthesis

Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
---
Swaps align for over 80 but get whole line this way

 security/apparmor/apparmorfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index d759c78..c8142cf 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -647,8 +647,7 @@ static struct aa_profile *__next_profile(struct aa_profile *p)
 		if (!list_entry_is_head(p, &parent->base.profiles, base.list))
 			return p;
 		p = parent;
-		parent = rcu_dereference_protected(parent->parent,
-					    mutex_is_locked(&parent->ns->lock));
+		parent = rcu_dereference_protected(parent->parent, mutex_is_locked(&parent->ns->lock));
 	}
 
 	/* is next another profile in the namespace */
-- 
2.1.4

[PATCH 6/7] security: apparmor: apparmorfs.c Comparison to NULL

[Walt Feasel]

Make style modifications for:
CHECK: Comparison to NULL could be written "!data"

Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
---
My first patch of this type checking if
this would be correct

 security/apparmor/apparmorfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index c8142cf..c3fff95 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -102,7 +102,7 @@ static char *aa_simple_write_to_buffer(int op, const char __user *userbuf,
 
 	/* freed by caller to simple_write_to_buffer */
 	data = kvmalloc(alloc_size);
-	if (data == NULL)
+	if (!data)
 		return ERR_PTR(-ENOMEM);
 
 	if (copy_from_user(data, userbuf, copy_size)) {
-- 
2.1.4

[PATCH 7/7] security: apparmor: apparmorfs.c Line continuations

[Walt Feasel]

Make style modifications for:
WARNING: Avoid unnecessary line continuations

Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
---
Swaps continuation for over 80 but on same line

 security/apparmor/apparmorfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index c3fff95..ee4f0d4 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -790,8 +790,7 @@ static const struct file_operations aa_fs_profiles_fops = {
 
 /** Base file system setup **/
 static struct aa_fs_entry aa_fs_entry_file[] = {
-	AA_FS_FILE_STRING("mask", "create read write exec append mmap_exec " \
-				  "link lock"),
+	AA_FS_FILE_STRING("mask", "create read write exec append mmap_exec", " link lock"),
 	{ }
 };
 
-- 
2.1.4

[PATCH 0/7] security: apparmor: apparmorfs.c Checkpatch mods

[Greg KH]

On Sat, Nov 26, 2016 at 03:43:18AM -0500, Walt Feasel wrote:
> Make checkpatch modifications to include:
> 
> Multiple blank lines
> Blank line after declarations
> Space after cast
> WARN_ON vs BUG_ON
> Align parenthesis
> Comparison to NULL
> Line continuations
> 
> Walt Feasel (7):
>   security: apparmor: apparmorfs.c Multiple blank lines
>   security: apparmor: apparmorfs.c Blank line after declarations
>   security: apparmor: apparmorfs.c Space after cast
>   security: apparmor: apparmorfs.c WARN_ON vs BUG_ON
>   security: apparmor: apparmorfs.c Align parenthesis
>   security: apparmor: apparmorfs.c Comparison to NULL
>   security: apparmor: apparmorfs.c Line continuations

Why are you sending apparmor patches to the kernelnewbies list and not
the maintainers and subsystem list for this code instead?

thanks,

greg k-h

[PATCH 0/7] security: apparmor: apparmorfs.c Checkpatch mods

[Walt Feasel]

On Sat, Nov 26, 2016 at 12:05:50PM +0100, Greg KH wrote:
> On Sat, Nov 26, 2016 at 03:43:18AM -0500, Walt Feasel wrote:
> > Make checkpatch modifications to include:
> > 
> > Multiple blank lines
> > Blank line after declarations
> > Space after cast
> > WARN_ON vs BUG_ON
> > Align parenthesis
> > Comparison to NULL
> > Line continuations
> > 
> > Walt Feasel (7):
> >   security: apparmor: apparmorfs.c Multiple blank lines
> >   security: apparmor: apparmorfs.c Blank line after declarations
> >   security: apparmor: apparmorfs.c Space after cast
> >   security: apparmor: apparmorfs.c WARN_ON vs BUG_ON
> >   security: apparmor: apparmorfs.c Align parenthesis
> >   security: apparmor: apparmorfs.c Comparison to NULL
> >   security: apparmor: apparmorfs.c Line continuations
> 
> Why are you sending apparmor patches to the kernelnewbies list and not
> the maintainers and subsystem list for this code instead?
> 
> thanks,
> 
> greg k-h
Because as stated in my patch notes I am not sure how to
fix certain cases.
I have not been able to find reference material other than
other patches and discussions for certain cases and have
to try and reason why.
You wanted me to move out of staging so I have looked to
other areas but do not wish to send in ridiculous patches
and have me seen as being malicious.
Since this is kernelnewbies mailing list isn't that it's
purpose or am I move on again?
Feel free to skip my post should they trouble you.

[PATCH 0/7] security: apparmor: apparmorfs.c Checkpatch mods

[Greg KH]

On Sat, Nov 26, 2016 at 11:56:17AM -0500, Walt Feasel wrote:
> On Sat, Nov 26, 2016 at 12:05:50PM +0100, Greg KH wrote:
> > On Sat, Nov 26, 2016 at 03:43:18AM -0500, Walt Feasel wrote:
> > > Make checkpatch modifications to include:
> > > 
> > > Multiple blank lines
> > > Blank line after declarations
> > > Space after cast
> > > WARN_ON vs BUG_ON
> > > Align parenthesis
> > > Comparison to NULL
> > > Line continuations
> > > 
> > > Walt Feasel (7):
> > >   security: apparmor: apparmorfs.c Multiple blank lines
> > >   security: apparmor: apparmorfs.c Blank line after declarations
> > >   security: apparmor: apparmorfs.c Space after cast
> > >   security: apparmor: apparmorfs.c WARN_ON vs BUG_ON
> > >   security: apparmor: apparmorfs.c Align parenthesis
> > >   security: apparmor: apparmorfs.c Comparison to NULL
> > >   security: apparmor: apparmorfs.c Line continuations
> > 
> > Why are you sending apparmor patches to the kernelnewbies list and not
> > the maintainers and subsystem list for this code instead?
> > 
> > thanks,
> > 
> > greg k-h
> Because as stated in my patch notes I am not sure how to
> fix certain cases.
> I have not been able to find reference material other than
> other patches and discussions for certain cases and have
> to try and reason why.
> You wanted me to move out of staging so I have looked to
> other areas but do not wish to send in ridiculous patches
> and have me seen as being malicious.
> Since this is kernelnewbies mailing list isn't that it's
> purpose or am I move on again?
> Feel free to skip my post should they trouble you.

Just send the patches to the proper people and list that
get_maintainer.pl shows you to, that's all, no need to bother
kernelnewbies with it.

good luck!

greg k-h

[PATCH 0/7] security: apparmor: apparmorfs.c Checkpatch mods

[Walt Feasel]

On Sat, Nov 26, 2016 at 06:19:22PM +0100, Greg KH wrote:
> On Sat, Nov 26, 2016 at 11:56:17AM -0500, Walt Feasel wrote:
> > On Sat, Nov 26, 2016 at 12:05:50PM +0100, Greg KH wrote:
> > > On Sat, Nov 26, 2016 at 03:43:18AM -0500, Walt Feasel wrote:
> > > > Make checkpatch modifications to include:
> > > > 
> > > > Multiple blank lines
> > > > Blank line after declarations
> > > > Space after cast
> > > > WARN_ON vs BUG_ON
> > > > Align parenthesis
> > > > Comparison to NULL
> > > > Line continuations
> > > > 
> > > > Walt Feasel (7):
> > > >   security: apparmor: apparmorfs.c Multiple blank lines
> > > >   security: apparmor: apparmorfs.c Blank line after declarations
> > > >   security: apparmor: apparmorfs.c Space after cast
> > > >   security: apparmor: apparmorfs.c WARN_ON vs BUG_ON
> > > >   security: apparmor: apparmorfs.c Align parenthesis
> > > >   security: apparmor: apparmorfs.c Comparison to NULL
> > > >   security: apparmor: apparmorfs.c Line continuations
> > > 
> > > Why are you sending apparmor patches to the kernelnewbies list and not
> > > the maintainers and subsystem list for this code instead?
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > Because as stated in my patch notes I am not sure how to
> > fix certain cases.
> > I have not been able to find reference material other than
> > other patches and discussions for certain cases and have
> > to try and reason why.
> > You wanted me to move out of staging so I have looked to
> > other areas but do not wish to send in ridiculous patches
> > and have me seen as being malicious.
> > Since this is kernelnewbies mailing list isn't that it's
> > purpose or am I move on again?
> > Feel free to skip my post should they trouble you.
> 
> Just send the patches to the proper people and list that
> get_maintainer.pl shows you to, that's all, no need to bother
> kernelnewbies with it.
> 
> good luck!
> 
> greg k-h
So kernelnewbies is not to be used to learn about how to fix
checkpatch type warning?

[PATCH 0/7] security: apparmor: apparmorfs.c Checkpatch mods

[Greg KH]

On Sat, Nov 26, 2016 at 01:02:14PM -0500, Walt Feasel wrote:
> On Sat, Nov 26, 2016 at 06:19:22PM +0100, Greg KH wrote:
> > On Sat, Nov 26, 2016 at 11:56:17AM -0500, Walt Feasel wrote:
> > > On Sat, Nov 26, 2016 at 12:05:50PM +0100, Greg KH wrote:
> > > > On Sat, Nov 26, 2016 at 03:43:18AM -0500, Walt Feasel wrote:
> > > > > Make checkpatch modifications to include:
> > > > > 
> > > > > Multiple blank lines
> > > > > Blank line after declarations
> > > > > Space after cast
> > > > > WARN_ON vs BUG_ON
> > > > > Align parenthesis
> > > > > Comparison to NULL
> > > > > Line continuations
> > > > > 
> > > > > Walt Feasel (7):
> > > > >   security: apparmor: apparmorfs.c Multiple blank lines
> > > > >   security: apparmor: apparmorfs.c Blank line after declarations
> > > > >   security: apparmor: apparmorfs.c Space after cast
> > > > >   security: apparmor: apparmorfs.c WARN_ON vs BUG_ON
> > > > >   security: apparmor: apparmorfs.c Align parenthesis
> > > > >   security: apparmor: apparmorfs.c Comparison to NULL
> > > > >   security: apparmor: apparmorfs.c Line continuations
> > > > 
> > > > Why are you sending apparmor patches to the kernelnewbies list and not
> > > > the maintainers and subsystem list for this code instead?
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > Because as stated in my patch notes I am not sure how to
> > > fix certain cases.
> > > I have not been able to find reference material other than
> > > other patches and discussions for certain cases and have
> > > to try and reason why.
> > > You wanted me to move out of staging so I have looked to
> > > other areas but do not wish to send in ridiculous patches
> > > and have me seen as being malicious.
> > > Since this is kernelnewbies mailing list isn't that it's
> > > purpose or am I move on again?
> > > Feel free to skip my post should they trouble you.
> > 
> > Just send the patches to the proper people and list that
> > get_maintainer.pl shows you to, that's all, no need to bother
> > kernelnewbies with it.
> > 
> > good luck!
> > 
> > greg k-h
> So kernelnewbies is not to be used to learn about how to fix
> checkpatch type warning?

Maybe, but really, if you have a well-formed patch, just send it to the
correct maintainers, it's up to them to accept it or not, that's their
job :)

greg k-h

[PATCH 0/7] security: apparmor: apparmorfs.c Checkpatch mods

[Bjørn Mork]

Greg KH <greg@kroah.com> writes:
> On Sat, Nov 26, 2016 at 01:02:14PM -0500, Walt Feasel wrote:
>
>> So kernelnewbies is not to be used to learn about how to fix
>> checkpatch type warning?
>
> Maybe, but really, if you have a well-formed patch, just send it to the
> correct maintainers, it's up to them to accept it or not, that's their
> job :)

And I think the most important reason for doing that is because only
they can answer the questions.  You need to know what the code does to
be able to answer things like "is it OK to replace BUG_ON with WARN_ON
here?". Although that is preferable according to checkpatch, it's not
necessarily a 1-to-1 replacement. The error path changes, and the
existing code is likely not tested or developed with the new path in
mind.

Nobody(?) in kernelnewbies knows anything about the apparmor code.  It's
not that we don't want to answer. We just can't.


Bj?rn

[PATCH 0/7] security: apparmor: apparmorfs.c Checkpatch mods

[Joel Fernandes]

On Nov 26, 2016 1:28 PM, "Bj?rn Mork" <bjorn@mork.no> wrote:
>
> Greg KH <greg@kroah.com> writes:
> > On Sat, Nov 26, 2016 at 01:02:14PM -0500, Walt Feasel wrote:
> >
> >> So kernelnewbies is not to be used to learn about how to fix
> >> checkpatch type warning?
> >
> > Maybe, but really, if you have a well-formed patch, just send it to the
> > correct maintainers, it's up to them to accept it or not, that's their
> > job :)
>
> And I think the most important reason for doing that is because only
> they can answer the questions.  You need to know what the code does to
> be able to answer things like "is it OK to replace BUG_ON with WARN_ON
> here?". Although that is preferable according to checkpatch, it's not
> necessarily a 1-to-1 replacement. The error path changes, and the
> existing code is likely not tested or developed with the new path in
> mind.
>
> Nobody(?) in kernelnewbies knows anything about the apparmor code.  It's
> not that we don't want to answer. We just can't.
>

I think a better approach is to document your thoughts possibly in
Documentation/ than posting random patches to teach people what YOU think
needs learning.

Also people can already, if they need to, learn from patches posted on LKML
about what THEY want to learn :).

Thanks,
Joel

>
> Bj?rn
>
>
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20161126/f95e227c/attachment.html