[v1] kcsan: Use min() to fix Coccinelle warning

[PATCH] kcsan: Use min() to fix Coccinelle warning

Posted by Thorsten Blum 1 year, 5 months ago

Fixes the following Coccinelle/coccicheck warning reported by
minmax.cocci:

	WARNING opportunity for min()

Use size_t instead of int for the result of min().

Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
 kernel/kcsan/debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/kcsan/debugfs.c b/kernel/kcsan/debugfs.c
index 1d1d1b0e4248..11b891fe6f7a 100644
--- a/kernel/kcsan/debugfs.c
+++ b/kernel/kcsan/debugfs.c
@@ -225,7 +225,7 @@ debugfs_write(struct file *file, const char __user *buf, size_t count, loff_t *o
 {
 	char kbuf[KSYM_NAME_LEN];
 	char *arg;
-	int read_len = count < (sizeof(kbuf) - 1) ? count : (sizeof(kbuf) - 1);
+	size_t read_len = min(count, (sizeof(kbuf) - 1));
 
 	if (copy_from_user(kbuf, buf, read_len))
 		return -EFAULT;
-- 
2.45.2

Re: [PATCH] kcsan: Use min() to fix Coccinelle warning

Posted by Marco Elver 1 year, 5 months ago

On Mon, 24 Jun 2024 at 00:08, Thorsten Blum <thorsten.blum@toblux.com> wrote:
>
> Fixes the following Coccinelle/coccicheck warning reported by
> minmax.cocci:
>
>         WARNING opportunity for min()
>
> Use size_t instead of int for the result of min().
>
> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>

Reviewed-by: Marco Elver <elver@google.com>

Thanks for polishing (but see below). Please compile-test with
CONFIG_KCSAN=y if you haven't.

> ---
>  kernel/kcsan/debugfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/kcsan/debugfs.c b/kernel/kcsan/debugfs.c
> index 1d1d1b0e4248..11b891fe6f7a 100644
> --- a/kernel/kcsan/debugfs.c
> +++ b/kernel/kcsan/debugfs.c
> @@ -225,7 +225,7 @@ debugfs_write(struct file *file, const char __user *buf, size_t count, loff_t *o
>  {
>         char kbuf[KSYM_NAME_LEN];
>         char *arg;
> -       int read_len = count < (sizeof(kbuf) - 1) ? count : (sizeof(kbuf) - 1);
> +       size_t read_len = min(count, (sizeof(kbuf) - 1));

While we're here polishing things this could be:

const size_t read_len = min(count, sizeof(kbuf) - 1);

( +const, remove redundant () )

>         if (copy_from_user(kbuf, buf, read_len))
>                 return -EFAULT;
> --
> 2.45.2

RE: [PATCH] kcsan: Use min() to fix Coccinelle warning

Posted by David Laight 1 year, 5 months ago

From: Marco Elver
> Sent: 24 June 2024 08:03
> >
> > Fixes the following Coccinelle/coccicheck warning reported by
> > minmax.cocci:
> >
> >         WARNING opportunity for min()
> >
> > Use size_t instead of int for the result of min().
> >
> > Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> 
> Reviewed-by: Marco Elver <elver@google.com>
> 
> Thanks for polishing (but see below). Please compile-test with
> CONFIG_KCSAN=y if you haven't.
> 
> > ---
> >  kernel/kcsan/debugfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/kcsan/debugfs.c b/kernel/kcsan/debugfs.c
> > index 1d1d1b0e4248..11b891fe6f7a 100644
> > --- a/kernel/kcsan/debugfs.c
> > +++ b/kernel/kcsan/debugfs.c
> > @@ -225,7 +225,7 @@ debugfs_write(struct file *file, const char __user *buf, size_t count, loff_t *o
> >  {
> >         char kbuf[KSYM_NAME_LEN];
> >         char *arg;
> > -       int read_len = count < (sizeof(kbuf) - 1) ? count : (sizeof(kbuf) - 1);
> > +       size_t read_len = min(count, (sizeof(kbuf) - 1));
> 
> While we're here polishing things this could be:
> 
> const size_t read_len = min(count, sizeof(kbuf) - 1);
> 
> ( +const, remove redundant () )

Pretty much no one makes variables 'const', it mostly just makes the code harder to read.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] kcsan: Use min() to fix Coccinelle warning

Posted by Marco Elver 1 year, 5 months ago

On Fri, 28 Jun 2024 at 16:52, David Laight <David.Laight@aculab.com> wrote:
>
> From: Marco Elver
> > Sent: 24 June 2024 08:03
> > >
> > > Fixes the following Coccinelle/coccicheck warning reported by
> > > minmax.cocci:
> > >
> > >         WARNING opportunity for min()
> > >
> > > Use size_t instead of int for the result of min().
> > >
> > > Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> >
> > Reviewed-by: Marco Elver <elver@google.com>
> >
> > Thanks for polishing (but see below). Please compile-test with
> > CONFIG_KCSAN=y if you haven't.
> >
> > > ---
> > >  kernel/kcsan/debugfs.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/kcsan/debugfs.c b/kernel/kcsan/debugfs.c
> > > index 1d1d1b0e4248..11b891fe6f7a 100644
> > > --- a/kernel/kcsan/debugfs.c
> > > +++ b/kernel/kcsan/debugfs.c
> > > @@ -225,7 +225,7 @@ debugfs_write(struct file *file, const char __user *buf, size_t count, loff_t *o
> > >  {
> > >         char kbuf[KSYM_NAME_LEN];
> > >         char *arg;
> > > -       int read_len = count < (sizeof(kbuf) - 1) ? count : (sizeof(kbuf) - 1);
> > > +       size_t read_len = min(count, (sizeof(kbuf) - 1));
> >
> > While we're here polishing things this could be:
> >
> > const size_t read_len = min(count, sizeof(kbuf) - 1);
> >
> > ( +const, remove redundant () )
>
> Pretty much no one makes variables 'const', it mostly just makes the code harder to read.

This is very much subjective. In my subjective opinion, it makes the
code easier to understand and it'll be harder to introduce accidental
mistakes. For trivial cases like this it really doesn't matter though.

Re: [PATCH] kcsan: Use min() to fix Coccinelle warning

Posted by Thorsten Blum 1 year, 5 months ago

On 24. Jun 2024, at 00:02, Marco Elver <elver@google.com> wrote:
> On Mon, 24 Jun 2024 at 00:08, Thorsten Blum <thorsten.blum@toblux.com> wrote:
>> 
>> Fixes the following Coccinelle/coccicheck warning reported by
>> minmax.cocci:
>> 
>>        WARNING opportunity for min()
>> 
>> Use size_t instead of int for the result of min().
>> 
>> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> 
> Reviewed-by: Marco Elver <elver@google.com>
> 
> Thanks for polishing (but see below). Please compile-test with
> CONFIG_KCSAN=y if you haven't.

Yes, I compile-tested it with CONFIG_KCSAN=y, but forgot to mention it.

> While we're here polishing things this could be:
> 
> const size_t read_len = min(count, sizeof(kbuf) - 1);
> 
> ( +const, remove redundant () )

Should I submit a v2 or are you adding this already?

Thanks,
Thorsten

Re: [PATCH] kcsan: Use min() to fix Coccinelle warning

Posted by Marco Elver 1 year, 5 months ago

On Mon, 24 Jun 2024 at 10:00, Thorsten Blum <thorsten.blum@toblux.com> wrote:
>
> On 24. Jun 2024, at 00:02, Marco Elver <elver@google.com> wrote:
> > On Mon, 24 Jun 2024 at 00:08, Thorsten Blum <thorsten.blum@toblux.com> wrote:
> >>
> >> Fixes the following Coccinelle/coccicheck warning reported by
> >> minmax.cocci:
> >>
> >>        WARNING opportunity for min()
> >>
> >> Use size_t instead of int for the result of min().
> >>
> >> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> >
> > Reviewed-by: Marco Elver <elver@google.com>
> >
> > Thanks for polishing (but see below). Please compile-test with
> > CONFIG_KCSAN=y if you haven't.
>
> Yes, I compile-tested it with CONFIG_KCSAN=y, but forgot to mention it.
>
> > While we're here polishing things this could be:
> >
> > const size_t read_len = min(count, sizeof(kbuf) - 1);
> >
> > ( +const, remove redundant () )
>
> Should I submit a v2 or are you adding this already?

Sending a v2 is cleaner, and also Cc Paul E. McKenney
<paulmck@kernel.org>, because the KCSAN patches go through the -rcu
kernel tree.

Thanks,
-- Marco