linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
@ 2021-11-07  3:17 Ajay Garg
  2021-11-08  7:03 ` Jiri Slaby
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Ajay Garg @ 2021-11-07  3:17 UTC (permalink / raw)
  To: gregkh, jirislaby, andriy.shevchenko, kernel, linux-serial, linux-kernel
  Cc: paskripkin, Ajay Garg

smatch-kchecker gives the following warnings when run on keyboard.c :

vt_do_kdgkb_ioctl() error: uninitialized symbol 'kbs'.
vt_do_kdgkb_ioctl() error: uninitialized symbol 'ret'.

This usually happens when switch has no default case and static 
analyzers and even sometimes compilers can’t prove that all possible 
values are covered.


Thus, the default switch-case has been added, which sets the values 
for the two variables :

        * kbs as NULL, which also nicely fits in with kfree.

        * ret as -ENOIOCTLCMD (on same lines if there is no cmd
                               match in "vt_do_kdskled" method).


Many thanks to the following for review of previous versions :

	* Pavel Skripkin <paskripkin@gmail.com> 
	* Andy Shevchenko <andy.shevchenko@gmail.com> 


Signed-off-by: Ajay Garg <ajaygargnsit@gmail.com>
---


There were discussions previously, and the current patch is the 
result.

v1 :
https://lore.kernel.org/linux-serial/YYZN30qfaKMskVwE@kroah.com/T/#t

v2 :
https://lore.kernel.org/linux-serial/CAHP4M8Vdj4Eb8q773BeHvsW9n6t=3n1WznuXAR4fZCNi1J6rOg@mail.gmail.com/T/#m18f45676feaba6b1f01ddd5fe607997b190ef4b9

v3 :
https://lore.kernel.org/linux-serial/20211106220315.392842-1-ajaygargnsit@gmail.com/T/#u

Changes in v2 :

        * Changes as required by scripts/checkpatch.pl

        * Checking whether kbs is not NULL before kfree is not required,
          as kfree(NULL) is safe. So, dropped the check.

Changes in v3 :

        * Using default-switch case, and setting the variables 
          when there is no matching cmd.

Changes in v4 :

        * Removed braces for the default switch-case.


 drivers/tty/vt/keyboard.c | 6 ++++++
 1 file changed, 6 insertions(+)


diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index c7fbbcdcc346..f66c32fe7ef1 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -2090,6 +2090,10 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 
 		ret = 0;
 		break;
+	default:
+		kbs = NULL;
+		ret = -ENOIOCTLCMD;
+		break;
 	}
 
 	kfree(kbs);

-- 
2.30.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
  2021-11-07  3:17 [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl Ajay Garg
@ 2021-11-08  7:03 ` Jiri Slaby
  2021-11-08  8:36 ` Andy Shevchenko
  2021-11-08  9:41 ` Johan Hovold
  2 siblings, 0 replies; 6+ messages in thread
From: Jiri Slaby @ 2021-11-08  7:03 UTC (permalink / raw)
  To: Ajay Garg, gregkh, andriy.shevchenko, kernel, linux-serial, linux-kernel
  Cc: paskripkin

On 07. 11. 21, 4:17, Ajay Garg wrote:
> smatch-kchecker gives the following warnings when run on keyboard.c :
> 
> vt_do_kdgkb_ioctl() error: uninitialized symbol 'kbs'.
> vt_do_kdgkb_ioctl() error: uninitialized symbol 'ret'.
> 
> This usually happens when switch has no default case and static
> analyzers and even sometimes compilers can’t prove that all possible
> values are covered.
> 
> 
> Thus, the default switch-case has been added, which sets the values
> for the two variables :

Just to shut up random static analyzers? Please don't.


-- 
js
suse labs

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
  2021-11-07  3:17 [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl Ajay Garg
  2021-11-08  7:03 ` Jiri Slaby
@ 2021-11-08  8:36 ` Andy Shevchenko
  2021-11-08  8:55   ` Ajay Garg
  2021-11-08  9:41 ` Johan Hovold
  2 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2021-11-08  8:36 UTC (permalink / raw)
  To: Ajay Garg
  Cc: Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko,
	Emil Renner Berthing, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List, Pavel Skripkin

On Sun, Nov 7, 2021 at 4:10 PM Ajay Garg <ajaygargnsit@gmail.com> wrote:
>
> smatch-kchecker gives the following warnings when run on keyboard.c :
>
> vt_do_kdgkb_ioctl() error: uninitialized symbol 'kbs'.
> vt_do_kdgkb_ioctl() error: uninitialized symbol 'ret'.
>
> This usually happens when switch has no default case and static
> analyzers and even sometimes compilers can’t prove that all possible
> values are covered.
>
>

One blank line is enough.

> Thus, the default switch-case has been added, which sets the values
> for the two variables :
>
>         * kbs as NULL, which also nicely fits in with kfree.
>
>         * ret as -ENOIOCTLCMD (on same lines if there is no cmd
>                                match in "vt_do_kdskled" method).
>
>
> Many thanks to the following for review of previous versions :
>
>         * Pavel Skripkin <paskripkin@gmail.com>
>         * Andy Shevchenko <andy.shevchenko@gmail.com>
>
>

Ditto for each such case.

...

> +       default:
> +               kbs = NULL;

> +               ret = -ENOIOCTLCMD;

Why is this? How is it supposed to work?

> +               break;
>         }

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
  2021-11-08  8:36 ` Andy Shevchenko
@ 2021-11-08  8:55   ` Ajay Garg
  0 siblings, 0 replies; 6+ messages in thread
From: Ajay Garg @ 2021-11-08  8:55 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko,
	Emil Renner Berthing, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List, Pavel Skripkin

> > This usually happens when switch has no default case and static
> > analyzers and even sometimes compilers can’t prove that all possible
> > values are covered.
> >
> >
>
> One blank line is enough.
>
> > Many thanks to the following for review of previous versions :
> >
> >         * Pavel Skripkin <paskripkin@gmail.com>
> >         * Andy Shevchenko <andy.shevchenko@gmail.com>
> >
> >
>
> Ditto for each such case.

Ok, will take care of this in all my future-patches.


>
> ...
>
> > +       default:
> > +               kbs = NULL;
>
> > +               ret = -ENOIOCTLCMD;
>
> Why is this? How is it supposed to work?

If there is no match for a cmd, causing the default-case to be hit, we
must return an error ret-code to the client.
The -ENOIOCTLCMD error ret-code has been chosen, on the same lines as
"vt_do_kdskled" method.


Thanks and Regards,
Ajay

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
  2021-11-07  3:17 [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl Ajay Garg
  2021-11-08  7:03 ` Jiri Slaby
  2021-11-08  8:36 ` Andy Shevchenko
@ 2021-11-08  9:41 ` Johan Hovold
  2021-11-08 13:51   ` Ajay Garg
  2 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2021-11-08  9:41 UTC (permalink / raw)
  To: Ajay Garg
  Cc: gregkh, jirislaby, andriy.shevchenko, kernel, linux-serial,
	linux-kernel, paskripkin

First, please fix your patch Subject which is way too verbose. You
should aim at less than 72 chars including prefix. Something like

	"vt: keyboard: suppress smatch warning in vt_do_kdgkb_ioctl"

should do.

On Sun, Nov 07, 2021 at 08:47:21AM +0530, Ajay Garg wrote:
> smatch-kchecker gives the following warnings when run on keyboard.c :
> 
> vt_do_kdgkb_ioctl() error: uninitialized symbol 'kbs'.
> vt_do_kdgkb_ioctl() error: uninitialized symbol 'ret'.
> 
> This usually happens when switch has no default case and static 
> analyzers and even sometimes compilers can’t prove that all possible 
> values are covered.
>
> Thus, the default switch-case has been added, which sets the values 
> for the two variables :
> 
>         * kbs as NULL, which also nicely fits in with kfree.
> 
>         * ret as -ENOIOCTLCMD (on same lines if there is no cmd
>                                match in "vt_do_kdskled" method).

Not sure how far we want to take the suppression of false-positive
warnings but at least this isn't the right way to do it.

> Many thanks to the following for review of previous versions :
> 
> 	* Pavel Skripkin <paskripkin@gmail.com> 
> 	* Andy Shevchenko <andy.shevchenko@gmail.com> 
> 
> 
> Signed-off-by: Ajay Garg <ajaygargnsit@gmail.com>
> ---
> 
> 
> There were discussions previously, and the current patch is the 
> result.
> 
> v1 :
> https://lore.kernel.org/linux-serial/YYZN30qfaKMskVwE@kroah.com/T/#t
> 
> v2 :
> https://lore.kernel.org/linux-serial/CAHP4M8Vdj4Eb8q773BeHvsW9n6t=3n1WznuXAR4fZCNi1J6rOg@mail.gmail.com/T/#m18f45676feaba6b1f01ddd5fe607997b190ef4b9
> 
> v3 :
> https://lore.kernel.org/linux-serial/20211106220315.392842-1-ajaygargnsit@gmail.com/T/#u
> 
> Changes in v2 :
> 
>         * Changes as required by scripts/checkpatch.pl
> 
>         * Checking whether kbs is not NULL before kfree is not required,
>           as kfree(NULL) is safe. So, dropped the check.
> 
> Changes in v3 :
> 
>         * Using default-switch case, and setting the variables 
>           when there is no matching cmd.
> 
> Changes in v4 :
> 
>         * Removed braces for the default switch-case.
> 
> 
>  drivers/tty/vt/keyboard.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index c7fbbcdcc346..f66c32fe7ef1 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -2090,6 +2090,10 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
>  
>  		ret = 0;
>  		break;
> +	default:
> +		kbs = NULL;
> +		ret = -ENOIOCTLCMD;
> +		break;
>  	}
>  
>  	kfree(kbs);

Instead, move the kfree() into the two cases blocks and initialise ret
to 0 as is done in several other vt helpers in case a driver bug ever
causes them to be called for the wrong cmds (e.g. instead of sprinkling
WARN_ON(1) in all those functions).

You may want to mention that the kfree warning was introduced by 

	07edff926520 ("vt: keyboard, reorder user buffer handling in vt_do_kdgkb_ioctl")

which moved the shared allocation into the switch statement, and perhaps
also mention

	4e1404a5cd04 ("vt: keyboard, extract and simplify vt_kdskbsent")

for the ret warning.

Johan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
  2021-11-08  9:41 ` Johan Hovold
@ 2021-11-08 13:51   ` Ajay Garg
  0 siblings, 0 replies; 6+ messages in thread
From: Ajay Garg @ 2021-11-08 13:51 UTC (permalink / raw)
  To: Johan Hovold
  Cc: gregkh, jirislaby, andriy.shevchenko, kernel, linux-serial,
	linux-kernel, paskripkin

Hi Johan,

Thanks for your time.

On Mon, Nov 8, 2021 at 3:11 PM Johan Hovold <johan@kernel.org> wrote:
>
> First, please fix your patch Subject which is way too verbose. You
> should aim at less than 72 chars including prefix. Something like
>
>         "vt: keyboard: suppress smatch warning in vt_do_kdgkb_ioctl"
>
> should do.

Thanks Johan, will keep this in mind in all my future patches.

Taking the subject as suggested by you for the current patch, after
removal of "smatch" keyword, as it otherwise produces :
WARNING: A patch subject line should describe the change not the tool
that found it
when run through checkpatch.pl



> >       kfree(kbs);
>
> Instead, move the kfree() into the two cases blocks

So, if I understand correctly,

* prior 07edff926520, the scope of kbs (allocation/deallocation) was
external to switch-cases.
* post 07edff926520, kbs is allocated internally for each case,
however the deallocation remains external.

Thanks for the much cleaner suggestion, will move kfree() into the two
cases blocks.



> and initialise ret to 0

Got it, other functions (except vt_do_kdskled) initialize ret to 0.
Thanks again.


> as is done in several other vt helpers in case a driver bug ever
> causes them to be called for the wrong cmds (e.g. instead of sprinkling
> WARN_ON(1) in all those functions).
>
> You may want to mention that the kfree warning was introduced by
>
>         07edff926520 ("vt: keyboard, reorder user buffer handling in vt_do_kdgkb_ioctl")
>
> which moved the shared allocation into the switch statement, and perhaps
> also mention
>
>         4e1404a5cd04 ("vt: keyboard, extract and simplify vt_kdskbsent")
>
> for the ret warning.

Thanks a ton Johan for your time in investigating the history.
This has made things, in the current scenario, crystal clear.


Have floated the v5 patch at :
https://lore.kernel.org/linux-serial/20211108134901.7449-1-ajaygargnsit@gmail.com/T/#u

Let's continue further discussion there.


Thanks and Regards,
Ajay

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-11-08 13:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-07  3:17 [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl Ajay Garg
2021-11-08  7:03 ` Jiri Slaby
2021-11-08  8:36 ` Andy Shevchenko
2021-11-08  8:55   ` Ajay Garg
2021-11-08  9:41 ` Johan Hovold
2021-11-08 13:51   ` Ajay Garg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).