linux-kernel - Re: [PATCH] watchdog: when watchdog_enabled is 0, let (soft,nmi)switch remain 1 after we read them in proc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=Vw1fG9sUiG4R6LiKnR2fgnt5Wr4QKzRg+v8MC15pWUEg@mail.gmail.com>
Date: Wed, 4 Sep 2024 12:38:40 -0700
From: Doug Anderson <dianders@...omium.org>
To: 张元瀚 Tio Zhang <tiozhang@...iglobal.com>
Cc: "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "tglx@...utronix.de" <tglx@...utronix.de>, 
	"zyhtheonly@...il.com" <zyhtheonly@...il.com>, "zyhtheonly@...h.net" <zyhtheonly@...h.net>, 
	"john.ogness@...utronix.de" <john.ogness@...utronix.de>, 
	"lizhe.67@...edance.com" <lizhe.67@...edance.com>, "mcgrof@...nel.org" <mcgrof@...nel.org>, 
	"linux@...ssschuh.net" <linux@...ssschuh.net>, "kjlx@...pleofstupid.com" <kjlx@...pleofstupid.com>, 
	"mingo@...hat.com" <mingo@...hat.com>, "peterz@...radead.org" <peterz@...radead.org>, 
	"juri.lelli@...hat.com" <juri.lelli@...hat.com>, 
	"vincent.guittot@...aro.org" <vincent.guittot@...aro.org>, 
	"dietmar.eggemann@....com" <dietmar.eggemann@....com>, "rostedt@...dmis.org" <rostedt@...dmis.org>, 
	"bsegall@...gle.com" <bsegall@...gle.com>, "mgorman@...e.de" <mgorman@...e.de>, 
	"bristot@...hat.com" <bristot@...hat.com>, "vschneid@...hat.com" <vschneid@...hat.com>
Subject: Re: [PATCH] watchdog: when watchdog_enabled is 0, let
 (soft,nmi)switch remain 1 after we read them in proc

Hi,

On Wed, Sep 4, 2024 at 2:23 AM 张元瀚 Tio Zhang <tiozhang@...iglobal.com> wrote:
>
> Hi,
>
> Ping :)
>
> // get_maintainer.pl does not tell who are the maintainers or reviewers of kernel/watchdog.c
> Add commit signers and sched maintainers to the CC list.

FWIW, people in the Linux community usually don't like "top posting"...

In any case, I saw your original patch but I struggled to make sense
of the explanation and so I left it for later and then never got back
to it. I suspect that the explanation needs to be a little clearer.


> On 8/22/24 下午3:09, "张元瀚 Tio Zhang" <tiozhang@...iglobal.com <mailto:tiozhang@...iglobal.com>> wrote:
>
>
> For users set "watchdog_user_enabled=0" but remaining
> "(soft,nmi)watchdog_user_enabled=1". Watchdog threads(,nmi watchdog)
> rework only if users reset "watchdog_user_enabled=1" without printing
> those watchdog swicthes. Otherwise (soft,nmi)watchdog_user_enabled
> will turn to 0 because of printing their values (It makes sense to print 0
> since they do not work any more, but it does not make sense to let user's
> swicthes change to 0 only by prints).
>
>
> And after that, watchdog only should work again by doing:
> (soft,nmi)watchdog_user_enabled=1
> *** can't print, or everything go back to 0 again ***
> watchdog_user_enabled=1
>
>
> So this patch fixes this situation:
>
>
> | name | value
> |----------------------------|--------------------------
> | watchdog_enabled | 0
> |----------------------------|--------------------------
> | nmi_watchdog_user_enabled | 1
> |----------------------------|--------------------------
> | soft_watchdog_user_enabled | 1
> |----------------------------|--------------------------
> | watchdog_user_enabled | 0
> |----------------------------|--------------------------
>
>
> cat /proc/sys/kernel/*watchdog
> |
> |
> V
> | name | value
> |----------------------------|--------------------------
> | watchdog_enabled | 0
> |----------------------------|--------------------------
> | nmi_watchdog_user_enabled | 0
> |----------------------------|--------------------------
> | soft_watchdog_user_enabled | 0
> |----------------------------|--------------------------
> | watchdog_user_enabled | 0
> |----------------------------|--------------------------
>
>
>
>
> Signed-off-by: Tio Zhang <tiozhang@...iglobal.com <mailto:tiozhang@...iglobal.com>>
> ---
> kernel/watchdog.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 51915b44ac73..42e69e83e76d 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -995,8 +995,18 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
> * On read synchronize the userspace interface. This is a
> * racy snapshot.
> */
> + old = READ_ONCE(*param);

Given that the first thing that both the "if" and "else" case do now
is to set "old" then it feels like it could move outside the "if"
statement.

Even though the existing code does it, I'm also a little doubtful that
a READ_ONCE() is really needed here. You're protected by a mutex so
you don't need to worry about other CPUs and it would be really
confusing to me if the compiler could optimize things in a way that
the READ_ONCE() was needed.


> *param = (watchdog_enabled & which) != 0;
> err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> + /*
> + * When "old" is 1 and watchdog_enabled is 0,
> + * it should not be change to 0 for printing
> + * nmi_watchdog_user_enabled or soft_watchdog_user_enabled.
> + * So after we print it as 0,
> + * we should recover it to 1.
> + */
> + if (old && !watchdog_enabled)
> + *param = old;

I'm confused. Why all this extra logic? This should just
_unconditionally_ restore "*param" to "old", right? The only reason
the code hacked "*param" in the first place was so that it could use
the common proc_dointvec_minmax() helper function. Aside from the
common helper function working there is never a reason to muck with
"*param" if "!write" so it should just always restore.

-Doug