[PATCH v2] selftests: watchdog: Validate optional file argument

[PATCH v2] selftests: watchdog: Validate optional file argument

[George G. Davis]

As reported by Eugeniu Rosca, a side of affect of commit c3f2490d6e92
("selftests: watchdog: Add optional file argument") is that arbitrary files
may be opened for watchdog testing, e.g.

./watchdog-test  -f /dev/zero
Watchdog Ticking Away!

To prevent watchdog-test from operating on non-watchdog device files,
validate that a file is indeed a watchdog device via an
ioctl(WDIOC_GETSUPPORT) call.

While we're at it, since the watchdog_info is available as a result of the
ioctl(WDIOC_GETSUPPORT) call, add a command line option to optionally show
the watchdog_info.

Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Signed-off-by: George G. Davis <george_davis@mentor.com>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
v1: Applied/tested on commit ce54eab71e210f ("kunit: fix failure to build without printk") of
    https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=next
v2: Squashed [1] and [2], and update commit description as discussed in [3].
    [1] https://patchwork.kernel.org/patch/11136283/
    [2] https://patchwork.kernel.org/patch/11136285/
    [3] https://patchwork.kernel.org/patch/11136285/#22883573
---
 tools/testing/selftests/watchdog/watchdog-test.c | 27 +++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
index afff120c7be6..f45e510500c0 100644
--- a/tools/testing/selftests/watchdog/watchdog-test.c
+++ b/tools/testing/selftests/watchdog/watchdog-test.c
@@ -19,7 +19,7 @@
 
 int fd;
 const char v = 'V';
-static const char sopts[] = "bdehp:t:Tn:NLf:";
+static const char sopts[] = "bdehp:t:Tn:NLf:i";
 static const struct option lopts[] = {
 	{"bootstatus",          no_argument, NULL, 'b'},
 	{"disable",             no_argument, NULL, 'd'},
@@ -32,6 +32,7 @@ static const struct option lopts[] = {
 	{"getpretimeout",       no_argument, NULL, 'N'},
 	{"gettimeleft",		no_argument, NULL, 'L'},
 	{"file",          required_argument, NULL, 'f'},
+	{"info",		no_argument, NULL, 'i'},
 	{NULL,                  no_argument, NULL, 0x0}
 };
 
@@ -72,6 +73,7 @@ static void usage(char *progname)
 	printf("Usage: %s [options]\n", progname);
 	printf(" -f, --file\t\tOpen watchdog device file\n");
 	printf("\t\t\tDefault is /dev/watchdog\n");
+	printf(" -i, --info\t\tShow watchdog_info\n");
 	printf(" -b, --bootstatus\tGet last boot status (Watchdog/POR)\n");
 	printf(" -d, --disable\t\tTurn off the watchdog timer\n");
 	printf(" -e, --enable\t\tTurn on the watchdog timer\n");
@@ -97,6 +99,7 @@ int main(int argc, char *argv[])
 	int c;
 	int oneshot = 0;
 	char *file = "/dev/watchdog";
+	struct watchdog_info info;
 
 	setbuf(stdout, NULL);
 
@@ -118,6 +121,16 @@ int main(int argc, char *argv[])
 		exit(-1);
 	}
 
+	/*
+	 * Validate that `file` is a watchdog device
+	 */
+	ret = ioctl(fd, WDIOC_GETSUPPORT, &info);
+	if (ret) {
+		printf("WDIOC_GETSUPPORT error '%s'\n", strerror(errno));
+		close(fd);
+		exit(ret);
+	}
+
 	optind = 0;
 
 	while ((c = getopt_long(argc, argv, sopts, lopts, NULL)) != -1) {
@@ -205,6 +218,18 @@ int main(int argc, char *argv[])
 		case 'f':
 			/* Handled above */
 			break;
+		case 'i':
+			/*
+			 * watchdog_info was obtained as part of file open
+			 * validation. So we just show it here.
+			 */
+			oneshot = 1;
+			printf("watchdog_info:\n");
+			printf(" identity:\t\t%s\n", info.identity);
+			printf(" firmware_version:\t%u\n",
+			       info.firmware_version);
+			printf(" options:\t\t%08x\n", info.options);
+			break;
 
 		default:
 			usage(argv[0]);
-- 
2.7.4

Re: [PATCH v2] selftests: watchdog: Validate optional file argument

[shuah]

On 9/16/19 12:49 PM, George G. Davis wrote:
> As reported by Eugeniu Rosca, a side of affect of commit c3f2490d6e92
> ("selftests: watchdog: Add optional file argument") is that arbitrary files
> may be opened for watchdog testing, e.g.
> 

You don't need to say this here since you are already have a
Reported-by tag. You are missing the Fixes tag.
> ./watchdog-test  -f /dev/zero
> Watchdog Ticking Away!
> 
> To prevent watchdog-test from operating on non-watchdog device files,
> validate that a file is indeed a watchdog device via an
> ioctl(WDIOC_GETSUPPORT) call.
> 
> While we're at it, since the watchdog_info is available as a result of the
> ioctl(WDIOC_GETSUPPORT) call, add a command line option to optionally show
> the watchdog_info.
> 

Let's try this again. I want two patches. The first one with Fixes tag.
The first patch might be candidate for going into stables.

The -i (info) should be a separate patch. This won't go into stables.

Please write a clear commit log. The following will help:

https://chris.beams.io/posts/git-commit/

> Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> v1: Applied/tested on commit ce54eab71e210f ("kunit: fix failure to build without printk") of
>      https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=next
> v2: Squashed [1] and [2], and update commit description as discussed in [3].
>      [1] https://patchwork.kernel.org/patch/11136283/
>      [2] https://patchwork.kernel.org/patch/11136285/
>      [3] https://patchwork.kernel.org/patch/11136285/#22883573
> ---
>   tools/testing/selftests/watchdog/watchdog-test.c | 27 +++++++++++++++++++++++-
>   1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
> index afff120c7be6..f45e510500c0 100644
> --- a/tools/testing/selftests/watchdog/watchdog-test.c
> +++ b/tools/testing/selftests/watchdog/watchdog-test.c
> @@ -19,7 +19,7 @@
>   
>   int fd;
>   const char v = 'V';
> -static const char sopts[] = "bdehp:t:Tn:NLf:";
> +static const char sopts[] = "bdehp:t:Tn:NLf:i";
>   static const struct option lopts[] = {
>   	{"bootstatus",          no_argument, NULL, 'b'},
>   	{"disable",             no_argument, NULL, 'd'},
> @@ -32,6 +32,7 @@ static const struct option lopts[] = {
>   	{"getpretimeout",       no_argument, NULL, 'N'},
>   	{"gettimeleft",		no_argument, NULL, 'L'},
>   	{"file",          required_argument, NULL, 'f'},
> +	{"info",		no_argument, NULL, 'i'},
>   	{NULL,                  no_argument, NULL, 0x0}
>   };
>   
> @@ -72,6 +73,7 @@ static void usage(char *progname)
>   	printf("Usage: %s [options]\n", progname);
>   	printf(" -f, --file\t\tOpen watchdog device file\n");
>   	printf("\t\t\tDefault is /dev/watchdog\n");
> +	printf(" -i, --info\t\tShow watchdog_info\n");
>   	printf(" -b, --bootstatus\tGet last boot status (Watchdog/POR)\n");
>   	printf(" -d, --disable\t\tTurn off the watchdog timer\n");
>   	printf(" -e, --enable\t\tTurn on the watchdog timer\n");
> @@ -97,6 +99,7 @@ int main(int argc, char *argv[])
>   	int c;
>   	int oneshot = 0;
>   	char *file = "/dev/watchdog";
> +	struct watchdog_info info;
>   
>   	setbuf(stdout, NULL);
>   
> @@ -118,6 +121,16 @@ int main(int argc, char *argv[])
>   		exit(-1);
>   	}
>   
> +	/*
> +	 * Validate that `file` is a watchdog device
> +	 */
> +	ret = ioctl(fd, WDIOC_GETSUPPORT, &info);
> +	if (ret) {
> +		printf("WDIOC_GETSUPPORT error '%s'\n", strerror(errno));
> +		close(fd);
> +		exit(ret);
> +	}
> +
>   	optind = 0;
>   
>   	while ((c = getopt_long(argc, argv, sopts, lopts, NULL)) != -1) {
> @@ -205,6 +218,18 @@ int main(int argc, char *argv[])
>   		case 'f':
>   			/* Handled above */
>   			break;
> +		case 'i':
> +			/*
> +			 * watchdog_info was obtained as part of file open
> +			 * validation. So we just show it here.
> +			 */
> +			oneshot = 1;
> +			printf("watchdog_info:\n");
> +			printf(" identity:\t\t%s\n", info.identity);
> +			printf(" firmware_version:\t%u\n",
> +			       info.firmware_version);
> +			printf(" options:\t\t%08x\n", info.options);
> +			break;
>   
>   		default:
>   			usage(argv[0]);
> 

thanks,
-- Shuah

Re: [PATCH v2] selftests: watchdog: Validate optional file argument

[Eugeniu Rosca]

Shuah,

On Mon, Sep 16, 2019 at 07:19:35PM -0600, shuah wrote:
> On 9/16/19 12:49 PM, George G. Davis wrote:
> > As reported by Eugeniu Rosca, a side of affect of commit c3f2490d6e92
> > ("selftests: watchdog: Add optional file argument") is that arbitrary files
> > may be opened for watchdog testing, e.g.
> > 
> 
> You don't need to say this here since you are already have a
> Reported-by tag. 

This looks like asking people to stick to your personal taste which
BTW doesn't really match the patterns established in Linux community.

With a bit of scripting, I am able to find around 4600 vanilla commits
which happen to mention the name of the reporter in addition to
Reported-by: https://paste.ubuntu.com/p/wNXfdGCJbX/ .

I really don't care if my name is mentioned once or twice, but I do
believe that requesting a new patch revision just based on this criteria
is nonsense. Can you please revise your review criteria?

> You are missing the Fixes tag.

The _fixed_ commit didn't land in vanilla as of v5.3-2061-gad062195731.
It is still undergoing the linux-next testing, where it can be found as
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c3f2490d6e92
("selftests: watchdog: Add optional file argument").

Since the _fixed_ commit is not yet in mainline, there is no Fixes tag
included in this patch.

> > ./watchdog-test  -f /dev/zero
> > Watchdog Ticking Away!
> > 
> > To prevent watchdog-test from operating on non-watchdog device files,
> > validate that a file is indeed a watchdog device via an
> > ioctl(WDIOC_GETSUPPORT) call.
> > 
> > While we're at it, since the watchdog_info is available as a result of the
> > ioctl(WDIOC_GETSUPPORT) call, add a command line option to optionally show
> > the watchdog_info.
> > 
> 
> Let's try this again. I want two patches. The first one with Fixes tag.
> The first patch might be candidate for going into stables.

This makes me wonder if you figured out the relationship and timeline
of this and the fixed patches. Since both are going to be part of the
same kernel release (v5.4), why do you worry about the stable updates?

> 
> The -i (info) should be a separate patch. This won't go into stables.

Please, see the above. I don't think this patch, or any of its parts,
are candidate for linux-stable.

> 
> Please write a clear commit log. The following will help:
> 
> https://chris.beams.io/posts/git-commit/

With all my appreciation for https://chris.beams.io/, I see no
contributions from him in Linux whatsoever. Given that Linux commit
descriptions and summary lines obey specific/unique rules, I doubt this
is the best guide to provide to your contributors :)

With the above inputs, can you please outline your expectations
precisely which changes are expected in the next patch revision, if at
all needed?

-- 
Best Regards,
Eugeniu

Re: [PATCH v2] selftests: watchdog: Validate optional file argument

[shuah]

On 9/17/19 8:54 AM, Eugeniu Rosca wrote:
> Shuah,
> 
> On Mon, Sep 16, 2019 at 07:19:35PM -0600, shuah wrote:
>> On 9/16/19 12:49 PM, George G. Davis wrote:
>>> As reported by Eugeniu Rosca, a side of affect of commit c3f2490d6e92
>>> ("selftests: watchdog: Add optional file argument") is that arbitrary files
>>> may be opened for watchdog testing, e.g.
>>>
>>
>> You don't need to say this here since you are already have a
>> Reported-by tag.
> 
> This looks like asking people to stick to your personal taste which
> BTW doesn't really match the patterns established in Linux community.
> 
> With a bit of scripting, I am able to find around 4600 vanilla commits
> which happen to mention the name of the reporter in addition to
> Reported-by: https://paste.ubuntu.com/p/wNXfdGCJbX/ .
> 
> I really don't care if my name is mentioned once or twice, but I do
> believe that requesting a new patch revision just based on this criteria
> is nonsense. Can you please revise your review criteria?

I already said what I want. I want two patches and the first one with
Fixes tag. The reason for that is that the first patch fixes a problem
in patch that is already in my tree which is fixes a problem.

I am going to mark the patch for stables and the first patch in this
series.

I would like the commit log written clearly. Having a clear commit log
is a critical review comment. It is important for any change to have
clear commit logs for clarity and maintainability.

So please send me two patches one with Fixes tag and second that has
the -i support.

thanks,
-- Shuah

Re: [PATCH v2] selftests: watchdog: Validate optional file argument

[Eugeniu Rosca]

Shuah,

On Tue, Sep 17, 2019 at 09:25:31AM -0600, shuah wrote:

[..]

> I want two patches and the first one with
> Fixes tag. 

I am not sure we are on the same page and you don't seem to be receptive
to what I say.

> The reason for that is that the first patch fixes a problem
> in patch that is already in my tree which is fixes a problem.

Here is my understanding of your request:

 +--------------+    +--------------+
 |1/2 this patch|    |1/2 this patch|
 |    (fix)     +----+  (feature)   |
 +------+-------+    +--------------+
        | Fixes
 +------v-------+
 |     [A]      |
 +------+-------+
        | Fixes
 +------v-------+
 |     [B]      |
 +--------------+

So, you ask to decompose this v2 patch into two parts (fix and feature),
__exactly like it was in v1__, with the reasoning that the bugfix
related part of this patch fixes [A] (which is true), while [A] fixes
another commit [B]. But given that [A] is a feature commit, adding brand
new functionality, there can't be any [B] commit being fixed by [A].

> I am going to mark the patch for stables and the first patch in this
> series.

I do not understand your request. Both current patch and [A] are
scheduled for v5.4. I do not see any relevant patches for linux-stable.
I hope either a clarification or a third opinion will shed more light
onto this totally unproductive dialogue.

[A] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c3f2490d6e92
    ("selftests: watchdog: Add optional file argument")
    https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=next&id=c3f2490d6e92
    ("selftests: watchdog: Add optional file argument")
[B] ???

-- 
Best Regards,
Eugeniu.

Re: [PATCH v2] selftests: watchdog: Validate optional file argument

[shuah]

On 9/17/19 10:54 AM, Eugeniu Rosca wrote:
> Shuah,
> 
> On Tue, Sep 17, 2019 at 09:25:31AM -0600, shuah wrote:
> 
> [..]
> 
>> I want two patches and the first one with
>> Fixes tag.
> 

These two patches need to be separate. The first one is a fix and
the second is an enhancement.

Please send two patches - the first one with Fixes tag.

thanks,
-- Shuah

Re: [PATCH v2] selftests: watchdog: Validate optional file argument

[Eugeniu Rosca]

On Tue, Sep 17, 2019 at 11:44:45AM -0600, shuah wrote:
> On 9/17/19 10:54 AM, Eugeniu Rosca wrote:
> > Shuah,
> > 
> > On Tue, Sep 17, 2019 at 09:25:31AM -0600, shuah wrote:
> > 
> > [..]
> > 
> > > I want two patches and the first one with
> > > Fixes tag.
> > 
> 
> These two patches need to be separate. The first one is a fix and
> the second is an enhancement.

That was exactly the idea of v1.

> 
> Please send two patches - the first one with Fixes tag.

Can you please pick [v1] series from below:
 - https://patchwork.kernel.org/patch/11136283/
 - https://patchwork.kernel.org/patch/11136285/

with adding a one-liner Fixes tag to the first patch?

-- 
Best Regards,
Eugeniu.

Re: [PATCH v2] selftests: watchdog: Validate optional file argument

[shuah]

On 9/17/19 11:50 AM, Eugeniu Rosca wrote:
> On Tue, Sep 17, 2019 at 11:44:45AM -0600, shuah wrote:
>> On 9/17/19 10:54 AM, Eugeniu Rosca wrote:
>>> Shuah,
>>>
>>> On Tue, Sep 17, 2019 at 09:25:31AM -0600, shuah wrote:
>>>
>>> [..]
>>>
>>>> I want two patches and the first one with
>>>> Fixes tag.
>>>
>>
>> These two patches need to be separate. The first one is a fix and
>> the second is an enhancement.
> 
> That was exactly the idea of v1.
> 
>>
>> Please send two patches - the first one with Fixes tag.
> 
> Can you please pick [v1] series from below:
>   - https://patchwork.kernel.org/patch/11136283/
>   - https://patchwork.kernel.org/patch/11136285/
> 
> with adding a one-liner Fixes tag to the first patch?
> 

Please send v2 with Fixes tag on the first one and making the changes
I suggested on that v1 series.

thanks,
-- Shuah

Re: [PATCH v2] selftests: watchdog: Validate optional file argument

[George G. Davis]

Hello Shuah,

On Tue, Sep 17, 2019 at 12:05:06PM -0600, shuah wrote:
> On 9/17/19 11:50 AM, Eugeniu Rosca wrote:
> >On Tue, Sep 17, 2019 at 11:44:45AM -0600, shuah wrote:
> >>On 9/17/19 10:54 AM, Eugeniu Rosca wrote:
> >>>Shuah,
> >>>
> >>>On Tue, Sep 17, 2019 at 09:25:31AM -0600, shuah wrote:
> >>>
> >>>[..]
> >>>
> >>>>I want two patches and the first one with
> >>>>Fixes tag.
> >>>
> >>
> >>These two patches need to be separate. The first one is a fix and
> >>the second is an enhancement.
> >
> >That was exactly the idea of v1.
> >
> >>
> >>Please send two patches - the first one with Fixes tag.
> >
> >Can you please pick [v1] series from below:
> >  - https://patchwork.kernel.org/patch/11136283/
> >  - https://patchwork.kernel.org/patch/11136285/
> >
> >with adding a one-liner Fixes tag to the first patch?
> >
> 
> Please send v2 with Fixes tag on the first one and making the changes
> I suggested on that v1 series.

I'll submit v3 series with the requested changes.

Thanks!

> 
> thanks,
> -- Shuah

-- 
Regards,
George

Re: [PATCH v2] selftests: watchdog: Validate optional file argument

[George G. Davis]

On Tue, Sep 17, 2019 at 02:32:47PM -0400, George G. Davis wrote:
> Hello Shuah,
> 
> On Tue, Sep 17, 2019 at 12:05:06PM -0600, shuah wrote:
> > On 9/17/19 11:50 AM, Eugeniu Rosca wrote:
> > >On Tue, Sep 17, 2019 at 11:44:45AM -0600, shuah wrote:
> > >>On 9/17/19 10:54 AM, Eugeniu Rosca wrote:
> > >>>Shuah,
> > >>>
> > >>>On Tue, Sep 17, 2019 at 09:25:31AM -0600, shuah wrote:
> > >>>
> > >>>[..]
> > >>>
> > >>>>I want two patches and the first one with
> > >>>>Fixes tag.
> > >>>
> > >>
> > >>These two patches need to be separate. The first one is a fix and
> > >>the second is an enhancement.
> > >
> > >That was exactly the idea of v1.
> > >
> > >>
> > >>Please send two patches - the first one with Fixes tag.
> > >
> > >Can you please pick [v1] series from below:
> > >  - https://patchwork.kernel.org/patch/11136283/
> > >  - https://patchwork.kernel.org/patch/11136285/
> > >
> > >with adding a one-liner Fixes tag to the first patch?
> > >
> > 
> > Please send v2 with Fixes tag on the first one and making the changes
> > I suggested on that v1 series.
> 
> I'll submit v3 series with the requested changes.

Eugeniu has submitted v3 with requested changes.

Thanks!

> 
> Thanks!
> 
> > 
> > thanks,
> > -- Shuah
> 
> -- 
> Regards,
> George

-- 
Regards,
George

Re: [PATCH v2] selftests: watchdog: Validate optional file argument

[Eugeniu Rosca]

(For LKML readability) Superseded by:
 - https://patchwork.kernel.org/patch/11149287/
   ("[v3,1/2] selftests: watchdog: Validate optional file argument")
 - https://patchwork.kernel.org/patch/11149289/
   ("[v3,2/2] selftests: watchdog: Add command line option to show watchdog_info")

-- 
Best Regards,
Eugeniu.