linux-kernel - Re: [PATCH v2] drop_caches: re-enable message after disabling

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] [day] [month] [year] [list]

Message-ID: <k742mxcj77qyga6gqo25yylne4ch3eqyluwplkc7utyqfydvlz@gga3m5vpdh2k>
Date: Mon, 17 Mar 2025 20:28:10 +0100
From: Joel Granados <joel.granados@...nel.org>
To: Ruiwu Chen <rwchen404@...il.com>
Cc: mcgrof@...nel.org, corbet@....net, keescook@...omium.org, 
	linux-doc@...r.kernel.org, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	viro@...iv.linux.org.uk, zachwade.k@...il.com
Subject: Re: [PATCH v2] drop_caches: re-enable message after disabling

On Fri, Mar 14, 2025 at 08:28:03PM +0800, Ruiwu Chen wrote:
> > On Wed, Mar 12, 2025 at 03:55:22PM -0700, Luis Chamberlain wrote:
> > > On Mon, Mar 10, 2025 at 02:51:11PM +0100, Joel Granados wrote:
> > > > On Sat, Mar 08, 2025 at 04:05:49PM +0800, Ruiwu Chen wrote:
> > > > > >> When 'echo 4 > /proc/sys/vm/drop_caches' the message is disabled,
> > > > > >> but there is no interface to enable the message, only by restarting
> > > > > >> the way, so add the 'echo 0 > /proc/sys/vm/drop_caches' way to
> > > > > >> enabled the message again.
> > > > > >> 
> > > > > >> Signed-off-by: Ruiwu Chen <rwchen404@...il.com>
> > > > > >
> > > > > > You are overcomplicating things, if you just want to re-enable messages
> > > > > > you can just use:
> > > > > >
> > > > > > -		stfu |= sysctl_drop_caches & 4;
> > > > > > +		stfu = sysctl_drop_caches & 4;
> > > > > >
> > > > > > The bool is there as 4 is intended as a bit flag, you can can figure
> > > > > > out what values you want and just append 4 to it to get the expected
> > > > > > result.
> > > > > >
> > > > > >  Luis
> > > > >
> > > > > Is that what you mean ?
> > > > >
> > > > > -               stfu |= sysctl_drop_caches & 4;
> > > > > +               stfu ^= sysctl_drop_caches & 4;
> > > > >
> > > > > 'echo 4 > /sys/kernel/vm/drop_caches' can disable or open messages,
> > > > > This is what I originally thought, but there is uncertainty that when different operators execute the command,
> > > > > It is not possible to determine whether this time is enabled or turned on unless you operate it twice.
> > > >
> > > > So can you use ^= or not?
> > > 
> > > No,  ^= does not work, see a boolean truth table.
> 
> I don't quite agree with you, you change this, 
> echo {1,2,3} will have the meaning of enable message
> 
> The initial logic:
> echo 1: free pagecache
> echo 2: free slab
> echo 3: free pagecache and slab
> echo 4: disable message
> 
> If you change it to something like this:
> stfu = sysctl_drop_caches & 4;
> echo 1: free pagecache  and enable message
> echo 2: free slab       and enable message
> echo 3: free pagecache  and enable message
As I read it, its should be
echo 3: free slab and pagecache     and enable message

> echo 4: disable message
This is a very good point. But the new logic does not shock me. I would
actually add some rows to your explanation in the following fashion:

echo 5: free pagecache            and disable message
echo 6: free slab                 and disable message
echo 7: free pagecache and slab   and disable message
echo 0: Nothing                   and enable message

This is in line with the file describing a binary value where every bit
position means something different. At the end its up to the maintainer
to decide what is "right".

> 
> echo 4 becomes meaningless, when echo 4 only the next message can be disabled
> Unable to continuously disable echo{1,2,3}
> 
> echo {1,2,3} always enabled the message
> echo {1,2,3} should not have the meaning of enabling messages
> 
> My thoughts:
> stfu ^= !!(sysctl_drop_caches & 4);
This is a bit convoluted. This is more understandable IMO:

  diff --git a/fs/drop_caches.c b/fs/drop_caches.c
  index d45ef541d848..15730593ae39 100644
  --- a/fs/drop_caches.c
  +++ b/fs/drop_caches.c
  @@ -68,12 +68,13 @@ int drop_caches_sysctl_handler(const struct ctl_table *table, int write,
                          drop_slab();
                          count_vm_event(DROP_SLAB);
                  }
  +               if (sysctl_drop_caches & 4)
  +                       stfu ^= 1;
                  if (!stfu) {
                          pr_info("%s (%d): drop_caches: %d\n",
                                  current->comm, task_pid_nr(current),
                                  sysctl_drop_caches);
                  }
  -               stfu |= sysctl_drop_caches & 4;
          }
          return 0;
   }

I'll add this to the patch that I sent in [1]. If you have any more
comments, please answer them in [1]

Best

[1] : https://lore.kernel.org/20250313-jag-drop_caches_msg-v1-1-c2e4e7874b72@kernel.org
-- 

Joel Granados