[PATCH] cgroup: add explicit cast and comment for return type conversion

[PATCH] cgroup: add explicit cast and comment for return type conversion

[Nicholas Mc Guire]

Type-checking coccinelle spatches are being used to locate type mismatches
between function signatures and return values in this case this produced:
./kernel/cgroup.c:2525 WARNING: return of wrong type
	ssize_t != size_t, 

Returning unsigned types converted to a signed type can be problematic
but in this case the size_t is <= PATH_MAX which is less than ulong/2 so
the conversion is safe - to make static code checking happy this is 
resolved by an explicit cast and appropriate comment.

Patch was compile tested with x86_64_defconfig (implies CONFIG_CGROUPS=y)

Patch is against 4.1-rc4 (localversion-next is -next-20150522)

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Not sure if "cleanups" like this are acceptable - in this case I did not
find any better way to make static code checkers happy though.

 kernel/cgroup.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index b91177f..04de621 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2523,7 +2523,11 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
 		sizeof(cgrp->root->release_agent_path));
 	spin_unlock(&release_agent_path_lock);
 	cgroup_kn_unlock(of->kn);
-	return nbytes;
+
+	/* the path of the release notifier is <= PATH_MAX
+	 * so "downsizing" to signed long is safe here
+	 */
+	return (ssize_t)nbytes;
 }
 
 static int cgroup_release_agent_show(struct seq_file *seq, void *v)
-- 
1.7.10.4

Re: [PATCH] cgroup: add explicit cast and comment for return type conversion

[Tejun Heo]

Hello,

On Sun, May 24, 2015 at 03:07:52PM +0200, Nicholas Mc Guire wrote:
> Type-checking coccinelle spatches are being used to locate type mismatches
> between function signatures and return values in this case this produced:
> ./kernel/cgroup.c:2525 WARNING: return of wrong type
> 	ssize_t != size_t, 
> 
> Returning unsigned types converted to a signed type can be problematic
> but in this case the size_t is <= PATH_MAX which is less than ulong/2 so
> the conversion is safe - to make static code checking happy this is 
> resolved by an explicit cast and appropriate comment.
> 
> Patch was compile tested with x86_64_defconfig (implies CONFIG_CGROUPS=y)
> 
> Patch is against 4.1-rc4 (localversion-next is -next-20150522)
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> 
> Not sure if "cleanups" like this are acceptable - in this case I did not
> find any better way to make static code checkers happy though.
> 
>  kernel/cgroup.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index b91177f..04de621 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2523,7 +2523,11 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
>  		sizeof(cgrp->root->release_agent_path));
>  	spin_unlock(&release_agent_path_lock);
>  	cgroup_kn_unlock(of->kn);
> -	return nbytes;
> +
> +	/* the path of the release notifier is <= PATH_MAX
> +	 * so "downsizing" to signed long is safe here
> +	 */
> +	return (ssize_t)nbytes;

idk, does this actually help anything?  This isn't different from any
other implicit type casts.  Are we gonna convert all downward implicit
casts to be explicit?

Thanks.

-- 
tejun

Re: [PATCH] cgroup: add explicit cast and comment for return type conversion

[Nicholas Mc Guire]

On Sun, 24 May 2015, Tejun Heo wrote:

> Hello,
> 
> On Sun, May 24, 2015 at 03:07:52PM +0200, Nicholas Mc Guire wrote:
> > Type-checking coccinelle spatches are being used to locate type mismatches
> > between function signatures and return values in this case this produced:
> > ./kernel/cgroup.c:2525 WARNING: return of wrong type
> > 	ssize_t != size_t, 
> > 
> > Returning unsigned types converted to a signed type can be problematic
> > but in this case the size_t is <= PATH_MAX which is less than ulong/2 so
> > the conversion is safe - to make static code checking happy this is 
> > resolved by an explicit cast and appropriate comment.
> > 
> > Patch was compile tested with x86_64_defconfig (implies CONFIG_CGROUPS=y)
> > 
> > Patch is against 4.1-rc4 (localversion-next is -next-20150522)
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> > 
> > Not sure if "cleanups" like this are acceptable - in this case I did not
> > find any better way to make static code checkers happy though.
> > 
> >  kernel/cgroup.c |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> > index b91177f..04de621 100644
> > --- a/kernel/cgroup.c
> > +++ b/kernel/cgroup.c
> > @@ -2523,7 +2523,11 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
> >  		sizeof(cgrp->root->release_agent_path));
> >  	spin_unlock(&release_agent_path_lock);
> >  	cgroup_kn_unlock(of->kn);
> > -	return nbytes;
> > +
> > +	/* the path of the release notifier is <= PATH_MAX
> > +	 * so "downsizing" to signed long is safe here
> > +	 */
> > +	return (ssize_t)nbytes;
> 
> idk, does this actually help anything?  This isn't different from any
> other implicit type casts.  Are we gonna convert all downward implicit
> casts to be explicit?
>
nop not downward but signed/unsigned  if it were down it would not be
a problem but signed/unsigned can be - for those cases where it can't
be fixed up by changing the declarations or return variable types 
explicit cast might make sense - as noted in the patch Im not sure either
if this form of cleanups is helpful. 

In the kernel core there are about 400 signed/unsigned implicit 
conversions (about 3k in the entire kernel) which is what Im trying to 
remove or if that is not possible in a resonable way mark as false positive.

thx!
hofrat

Re: [PATCH] cgroup: add explicit cast and comment for return type conversion

[Tejun Heo]

Hello, Nicholas.

On Mon, May 25, 2015 at 07:57:42AM +0200, Nicholas Mc Guire wrote:
> nop not downward but signed/unsigned  if it were down it would not be
> a problem but signed/unsigned can be - for those cases where it can't
> be fixed up by changing the declarations or return variable types 
> explicit cast might make sense - as noted in the patch Im not sure either
> if this form of cleanups is helpful. 
> 
> In the kernel core there are about 400 signed/unsigned implicit 
> conversions (about 3k in the entire kernel) which is what Im trying to 
> remove or if that is not possible in a resonable way mark as false positive.

I still don't get it.  What does this buy us actually?  If we continue
to do this, people would just learn to add explicit cast when doing
sign conversions.  We just converge to a different behavior without
actually gaining any protection.  What's the benefit of doing this?

Thanks.

-- 
tejun

Re: [PATCH] cgroup: add explicit cast and comment for return type conversion

[Nicholas Mc Guire]

On Mon, 25 May 2015, Tejun Heo wrote:

> Hello, Nicholas.
> 
> On Mon, May 25, 2015 at 07:57:42AM +0200, Nicholas Mc Guire wrote:
> > nop not downward but signed/unsigned  if it were down it would not be
> > a problem but signed/unsigned can be - for those cases where it can't
> > be fixed up by changing the declarations or return variable types 
> > explicit cast might make sense - as noted in the patch Im not sure either
> > if this form of cleanups is helpful. 
> > 
> > In the kernel core there are about 400 signed/unsigned implicit 
> > conversions (about 3k in the entire kernel) which is what Im trying to 
> > remove or if that is not possible in a resonable way mark as false positive.
> 
> I still don't get it.  What does this buy us actually?  If we continue
> to do this, people would just learn to add explicit cast when doing
> sign conversions.  We just converge to a different behavior without
> actually gaining any protection.  What's the benefit of doing this?
>
that would be no benefit of course - the goal is not to simply put casts
in but to use casts as last resort if type cleanups are not doable or if
the type missmatch is intended - the cast then should document that it
is intentional and comments explain why it is justified. If that were the
result of type cleanup I think it would benefit the kernel code as I 
suspect that quite a few of the type missmatches simply happened.

thx!
hofrat

Re: [PATCH] cgroup: add explicit cast and comment for return type conversion

[Tejun Heo]

Hello, Nicholas.

On Mon, May 25, 2015 at 01:50:47PM +0200, Nicholas Mc Guire wrote:
> that would be no benefit of course - the goal is not to simply put casts
> in but to use casts as last resort if type cleanups are not doable or if
> the type missmatch is intended - the cast then should document that it
> is intentional and comments explain why it is justified. If that were the
> result of type cleanup I think it would benefit the kernel code as I 
> suspect that quite a few of the type missmatches simply happened.

I'm having a bit of hard time agreeing with the utility of this.  If
you can fix up the variable type to go away, sure, but adding
unnecessary explicit cast and comment for something this trivial?  I'm
not sure.  I mean, C is not a language which can propagate param
constraints to the return types.  e.g. strnlen() will happily return
size_t even when the maximum length is e.g. int.  We simply aren't
writing in a language where these things are easily distinguished and
I'm not sure shoehorning explicit constraints all over the source code
brings enough benefit to justify the added noise.

If you can identify actual problem cases, awesome.  If some can easily
be removed by tweaking types to match the actual usage, great too, but
let's please not do this explicit version of implicit casts and
pointless comments.

Thanks.

-- 
tejun

Re: [PATCH] cgroup: add explicit cast and comment for return type conversion

[Nicholas Mc Guire]

On Mon, 25 May 2015, Tejun Heo wrote:

> Hello, Nicholas.
> 
> On Mon, May 25, 2015 at 01:50:47PM +0200, Nicholas Mc Guire wrote:
> > that would be no benefit of course - the goal is not to simply put casts
> > in but to use casts as last resort if type cleanups are not doable or if
> > the type missmatch is intended - the cast then should document that it
> > is intentional and comments explain why it is justified. If that were the
> > result of type cleanup I think it would benefit the kernel code as I 
> > suspect that quite a few of the type missmatches simply happened.
> 
> I'm having a bit of hard time agreeing with the utility of this.  If
> you can fix up the variable type to go away, sure, but adding
> unnecessary explicit cast and comment for something this trivial?  I'm
> not sure.  I mean, C is not a language which can propagate param
> constraints to the return types.  e.g. strnlen() will happily return
> size_t even when the maximum length is e.g. int.  We simply aren't
> writing in a language where these things are easily distinguished and
> I'm not sure shoehorning explicit constraints all over the source code
> brings enough benefit to justify the added noise.
> 
> If you can identify actual problem cases, awesome.  If some can easily
> be removed by tweaking types to match the actual usage, great too, but
> let's please not do this explicit version of implicit casts and
> pointless comments.
>
got it - not an issue for me - as noted I was not that sure how 
sensible it is either the point of this RFC was precisely to 
clarify this. Will mark those safe conversions as false-postives
then and leave it as is.

Thanks for the clarification!

thx!
hofrat