linux-kernel - Re: [PATCH] pipe: don't update {a,c,m}time for anonymous pipes

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: <CAGudoHGptAB1C+vKpfoYo+S9tU2Ow2LWbQeyHKwBpzy9Xh_b=w@mail.gmail.com>
Date: Tue, 4 Feb 2025 15:21:18 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Christian Brauner <brauner@...nel.org>, Linus Torvalds <torvalds@...ux-foundation.org>, 
	David Howells <dhowells@...hat.com>, "Gautham R. Shenoy" <gautham.shenoy@....com>, 
	K Prateek Nayak <kprateek.nayak@....com>, Neeraj Upadhyay <Neeraj.Upadhyay@....com>, 
	Oliver Sang <oliver.sang@...el.com>, Swapnil Sapkal <swapnil.sapkal@....com>, 
	WangYuli <wangyuli@...ontech.com>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pipe: don't update {a,c,m}time for anonymous pipes

On Tue, Feb 4, 2025 at 2:22 PM Oleg Nesterov <oleg@...hat.com> wrote:
> These numbers are visible in fstat() but hopefully nobody uses this
> information and file_accessed/file_update_time are not that cheap.

I'll note majority of the problem most likely stems from
mnt_get_write_access rolling with:
        preempt_disable();
        mnt_inc_writers(mnt);
        /*
         * The store to mnt_inc_writers must be visible before we pass
         * MNT_WRITE_HOLD loop below, so that the slowpath can see our
         * incremented count after it has set MNT_WRITE_HOLD.
         */
        smp_mb();

Unfortunately as is the MNT_WRITE_HOLD thing gets set under a
spinlock, so it may be this will be hard to rework in the same vein in
which percpu rwsems are operating, but perhaps someone(tm) may take an
honest shot?

Would be of benefit everywhere and possibly obsolete this patch.

> +       if (ret > 0 && !is_pipe_inode(file_inode(filp))) {

Total nit in case there is a v2:

ret is typically > 0 and most of the time this is going to be an
anonymous pipe, so I would swap these conditions around.

A not nit is that "is_pipe_inode" is imo misleading -- a named pipe
inode is also a pipe inode. How about "is_anon_pipe"?

-- 
Mateusz Guzik <mjguzik gmail.com>