Re: [PATCH 1/2] kernfs: dont take i_lock on inode attr read

From: Ian Kent
Date: Sun Jan 22 2023 - 22:12:14 EST



On 29/12/22 21:07, Ian Kent wrote:

On 29/12/22 17:20, Arnd Bergmann wrote:
On Fri, Dec 23, 2022, at 00:11, Ian Kent wrote:
On 21/12/22 21:34, Anders Roxell wrote:
On 2022-10-31 12:30, Tejun Heo wrote:
On Tue, Oct 18, 2022 at 10:32:42AM +0800, Ian Kent wrote:
The kernfs write lock is held when the kernfs node inode attributes
are updated. Therefore, when either kernfs_iop_getattr() or
kernfs_iop_permission() are called the kernfs node inode attributes
won't change.

Consequently concurrent kernfs_refresh_inode() calls always copy the
same values from the kernfs node.

So there's no need to take the inode i_lock to get consistent values
for generic_fillattr() and generic_permission(), the kernfs read lock
is sufficient.

Signed-off-by: Ian Kent <raven@xxxxxxxxxx>
Acked-by: Tejun Heo <tj@xxxxxxxxxx>
Hi,

Building an allmodconfig arm64 kernel on yesterdays next-20221220 and
booting that in qemu I see the following "BUG: KCSAN: data-race in
set_nlink / set_nlink".

I'll check if I missed any places where set_link() could be
called where the link count could be different.


If there aren't any the question will then be can writing the
same value to this location in multiple concurrent threads
corrupt it?
I think the race that is getting reported for set_nlink()
is about this bit getting called simulatenously on multiple
CPUs with only the read lock held for the inode:

      /* Yes, some filesystems do change nlink from zero to one */
      if (inode->i_nlink == 0)
atomic_long_dec(&inode->i_sb->s_remove_count);
      inode->__i_nlink = nlink;

Since i_nlink and __i_nlink refer to the same memory location,
the 'inode->i_nlink == 0' check can be true for all of them
before the nonzero nlink value gets set, and this results in
s_remove_count being decremented more than once.


Thanks for the comment Arnd.


Hello all,


I've been looking at this and after consulting Miklos and his pointing

out that it looks like a false positive the urgency dropped off a bit. So

apologies for taking so long to report back.


Anyway it needs some description of conclusions reached so far.


I'm still looking around but in short, kernfs will set directories to <# of

directory entries> + 2 unconditionally for directories. I can't yet find

any other places where i_nlink is set or changed and if there are none

then i_nlink will never be set to zero so the race should not occur.


Consequently my claim is this is a real false positive.


There are the file system operations that may be passed at mount time

but given the way kernfs sets i_nlink it pretty much dictates those operations

(if there were any that modify it and there don't appear to be any) leave it

alone.


So it just doesn't make sense for users of kernfs to fiddle with i_nlink ...


Ian