* Switching to iterate_shared
[not found] ` <CAHk-=wgkNwDikLfEkqLxCWR=pLi1rbPZ5eyE8FbfmXP2=r3qcw@mail.gmail.com>
@ 2022-08-16 19:11 ` Matthew Wilcox
2022-08-16 22:30 ` Casey Schaufler
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Matthew Wilcox @ 2022-08-16 19:11 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, linux-fsdevel, ceph-devel, coda, codalist, Namjae Jeon,
Sungjong Seo, jfs-discussion, ocfs2-devel, devel, linux-unionfs,
linux-security-module, apparmor, Hans de Goede
On Tue, Aug 16, 2022 at 11:58:36AM -0700, Linus Torvalds wrote:
> That said, our filldir code is still confusing as hell. And I would
> really like to see that "shared vs non-shared" iterator thing go away,
> with everybody using the shared one - and filesystems that can't deal
> with it using their own lock.
>
> But that's a completely independent wart in our complicated filldir saga.
>
> But if somebody were to look at that iterate-vs-iterate_shared, that
> would be lovely. A quick grep shows that we don't have *that* many of
> the non-shared cases left:
>
> git grep '\.iterate\>.*='
>
> seems to imply that converting them to a "use my own load" wouldn't be
> _too_ bad.
>
> And some of them might actually be perfectly ok with the shared
> semantics (ie inode->i_rwsem held just for reading) and they just were
> never converted originally.
What's depressing is that some of these are newly added. It'd be
great if we could attach something _like_ __deprecated to things
that checkpatch could pick up on.
fs/adfs/dir_f.c: .iterate = adfs_f_iterate,
fs/adfs/dir_fplus.c: .iterate = adfs_fplus_iterate,
ADFS is read-only, so must be safe?
fs/ceph/dir.c: .iterate = ceph_readdir,
fs/ceph/dir.c: .iterate = ceph_readdir,
At least CEPH has active maintainers, cc'd
fs/coda/dir.c: .iterate = coda_readdir,
Would anyone notice if we broke CODA? Maintainers cc'd anyway.
fs/exfat/dir.c: .iterate = exfat_iterate,
Exfat is a new addition, but has active maintainers.
fs/jfs/namei.c: .iterate = jfs_readdir,
Maintainer cc'd
fs/ntfs/dir.c: .iterate = ntfs_readdir, /* Read directory contents. */
Maybe we can get rid of ntfs soon.
fs/ocfs2/file.c: .iterate = ocfs2_readdir,
fs/ocfs2/file.c: .iterate = ocfs2_readdir,
maintainers cc'd
fs/orangefs/dir.c: .iterate = orangefs_dir_iterate,
New; maintainer cc'd
fs/overlayfs/readdir.c: .iterate = ovl_iterate,
Active maintainer, cc'd
fs/proc/base.c: .iterate = proc_##LSM##_attr_dir_iterate, \
Hmm. We need both SMACK and Apparmor to agree to this ... cc's added.
fs/vboxsf/dir.c: .iterate = vboxsf_dir_iterate,
Also newly added. Maintainer cc'd.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Switching to iterate_shared
2022-08-16 19:11 ` Switching to iterate_shared Matthew Wilcox
@ 2022-08-16 22:30 ` Casey Schaufler
2022-08-16 23:09 ` Linus Torvalds
2022-08-18 14:35 ` Matthew Wilcox
2022-08-18 16:14 ` [apparmor] " John Johansen
2 siblings, 1 reply; 5+ messages in thread
From: Casey Schaufler @ 2022-08-16 22:30 UTC (permalink / raw)
To: Matthew Wilcox, Linus Torvalds
Cc: Al Viro, linux-fsdevel, ceph-devel, coda, codalist, Namjae Jeon,
Sungjong Seo, jfs-discussion, ocfs2-devel, devel, linux-unionfs,
linux-security-module, apparmor, Hans de Goede, casey
On 8/16/2022 12:11 PM, Matthew Wilcox wrote:
> On Tue, Aug 16, 2022 at 11:58:36AM -0700, Linus Torvalds wrote:
>> That said, our filldir code is still confusing as hell. And I would
>> really like to see that "shared vs non-shared" iterator thing go away,
>> with everybody using the shared one - and filesystems that can't deal
>> with it using their own lock.
>>
>> But that's a completely independent wart in our complicated filldir saga.
>>
>> But if somebody were to look at that iterate-vs-iterate_shared, that
>> would be lovely. A quick grep shows that we don't have *that* many of
>> the non-shared cases left:
>>
>> git grep '\.iterate\>.*='
>>
>> seems to imply that converting them to a "use my own load" wouldn't be
>> _too_ bad.
>>
>> And some of them might actually be perfectly ok with the shared
>> semantics (ie inode->i_rwsem held just for reading) and they just were
>> never converted originally.
> What's depressing is that some of these are newly added. It'd be
> great if we could attach something _like_ __deprecated to things
> that checkpatch could pick up on.
>
> fs/adfs/dir_f.c: .iterate = adfs_f_iterate,
> fs/adfs/dir_fplus.c: .iterate = adfs_fplus_iterate,
>
> ADFS is read-only, so must be safe?
>
> fs/ceph/dir.c: .iterate = ceph_readdir,
> fs/ceph/dir.c: .iterate = ceph_readdir,
>
> At least CEPH has active maintainers, cc'd
>
> fs/coda/dir.c: .iterate = coda_readdir,
>
> Would anyone notice if we broke CODA? Maintainers cc'd anyway.
>
> fs/exfat/dir.c: .iterate = exfat_iterate,
>
> Exfat is a new addition, but has active maintainers.
>
> fs/jfs/namei.c: .iterate = jfs_readdir,
>
> Maintainer cc'd
>
> fs/ntfs/dir.c: .iterate = ntfs_readdir, /* Read directory contents. */
>
> Maybe we can get rid of ntfs soon.
>
> fs/ocfs2/file.c: .iterate = ocfs2_readdir,
> fs/ocfs2/file.c: .iterate = ocfs2_readdir,
>
> maintainers cc'd
>
> fs/orangefs/dir.c: .iterate = orangefs_dir_iterate,
>
> New; maintainer cc'd
>
> fs/overlayfs/readdir.c: .iterate = ovl_iterate,
>
> Active maintainer, cc'd
>
> fs/proc/base.c: .iterate = proc_##LSM##_attr_dir_iterate, \
>
> Hmm. We need both SMACK and Apparmor to agree to this ... cc's added.
Smack passes all tests and seems perfectly content with the change.
I can't say that the tests stress this interface.
>
> fs/vboxsf/dir.c: .iterate = vboxsf_dir_iterate,
>
> Also newly added. Maintainer cc'd.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Switching to iterate_shared
2022-08-16 22:30 ` Casey Schaufler
@ 2022-08-16 23:09 ` Linus Torvalds
0 siblings, 0 replies; 5+ messages in thread
From: Linus Torvalds @ 2022-08-16 23:09 UTC (permalink / raw)
To: Casey Schaufler
Cc: Matthew Wilcox, Al Viro, linux-fsdevel, ceph-devel, coda,
codalist, Namjae Jeon, Sungjong Seo, jfs-discussion, ocfs2-devel,
devel, linux-unionfs, linux-security-module, apparmor,
Hans de Goede
On Tue, Aug 16, 2022 at 3:30 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Smack passes all tests and seems perfectly content with the change.
> I can't say that the tests stress this interface.
All the security filesystems really seem to boil down to just calling
that 'proc_pident_readdir()' function with different sets of 'const
struct pid_entry' arrays.
And all that does is to make sure the pidents are filled in by that
proc_fill_cache(), which basically does a filename lookup.
And a filename lookup *already* has to be able to handle being called
in parallel, because that's how filename lookup works:
[.. miss in dcache ..]
lookup_slow ->
inode_lock_shared(dir);
__lookup_slow -> does the
inode_unlock_shared(dir);
so as long as the proc_fill_cache() handles the d_in_lookup()
situation correctly (where we serialize on one single _name_ in the
directory), that should all be good.
And proc_fill_cache() does indeed seem to handle it right - and if it
didn't, it would be fundamentally racy with regular lookups - so I
think all those security layer proc_##LSM##_attr_dir_iterate cases can
be moved over to iterate_shared with no code change.
But again, maybe there's something really subtle I'm overlooking. Or
maybe not something subtle at all, and I'm just missing a big honking
issue.
Linus
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Switching to iterate_shared
2022-08-16 19:11 ` Switching to iterate_shared Matthew Wilcox
2022-08-16 22:30 ` Casey Schaufler
@ 2022-08-18 14:35 ` Matthew Wilcox
2022-08-18 16:14 ` [apparmor] " John Johansen
2 siblings, 0 replies; 5+ messages in thread
From: Matthew Wilcox @ 2022-08-18 14:35 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, linux-fsdevel, ceph-devel, coda, codalist, Namjae Jeon,
Sungjong Seo, jfs-discussion, ocfs2-devel, devel, linux-unionfs,
linux-security-module, apparmor, Hans de Goede
On Tue, Aug 16, 2022 at 08:11:31PM +0100, Matthew Wilcox wrote:
> fs/adfs/dir_f.c: .iterate = adfs_f_iterate,
> fs/adfs/dir_fplus.c: .iterate = adfs_fplus_iterate,
>
> ADFS is read-only, so must be safe?
I just checked ADFS. This isn't a f_ops ->iterate, this is a special
adfs_dir_ops. ADFS already uses f_ops->iterate_shared.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [apparmor] Switching to iterate_shared
2022-08-16 19:11 ` Switching to iterate_shared Matthew Wilcox
2022-08-16 22:30 ` Casey Schaufler
2022-08-18 14:35 ` Matthew Wilcox
@ 2022-08-18 16:14 ` John Johansen
2 siblings, 0 replies; 5+ messages in thread
From: John Johansen @ 2022-08-18 16:14 UTC (permalink / raw)
To: Matthew Wilcox, Linus Torvalds
Cc: jfs-discussion, Hans de Goede, devel, apparmor, linux-unionfs,
codalist, coda, linux-security-module, Al Viro, linux-fsdevel,
ceph-devel, Sungjong Seo, Namjae Jeon, ocfs2-devel
On 8/16/22 12:11, Matthew Wilcox wrote:
> On Tue, Aug 16, 2022 at 11:58:36AM -0700, Linus Torvalds wrote:
>> That said, our filldir code is still confusing as hell. And I would
>> really like to see that "shared vs non-shared" iterator thing go away,
>> with everybody using the shared one - and filesystems that can't deal
>> with it using their own lock.
>>
>> But that's a completely independent wart in our complicated filldir saga.
>>
>> But if somebody were to look at that iterate-vs-iterate_shared, that
>> would be lovely. A quick grep shows that we don't have *that* many of
>> the non-shared cases left:
>>
>> git grep '\.iterate\>.*='
>>
>> seems to imply that converting them to a "use my own load" wouldn't be
>> _too_ bad.
>>
>> And some of them might actually be perfectly ok with the shared
>> semantics (ie inode->i_rwsem held just for reading) and they just were
>> never converted originally.
>
> What's depressing is that some of these are newly added. It'd be
> great if we could attach something _like_ __deprecated to things
> that checkpatch could pick up on.
>
> fs/adfs/dir_f.c: .iterate = adfs_f_iterate,
> fs/adfs/dir_fplus.c: .iterate = adfs_fplus_iterate,
>
> ADFS is read-only, so must be safe?
>
> fs/ceph/dir.c: .iterate = ceph_readdir,
> fs/ceph/dir.c: .iterate = ceph_readdir,
>
> At least CEPH has active maintainers, cc'd
>
> fs/coda/dir.c: .iterate = coda_readdir,
>
> Would anyone notice if we broke CODA? Maintainers cc'd anyway.
>
> fs/exfat/dir.c: .iterate = exfat_iterate,
>
> Exfat is a new addition, but has active maintainers.
>
> fs/jfs/namei.c: .iterate = jfs_readdir,
>
> Maintainer cc'd
>
> fs/ntfs/dir.c: .iterate = ntfs_readdir, /* Read directory contents. */
>
> Maybe we can get rid of ntfs soon.
>
> fs/ocfs2/file.c: .iterate = ocfs2_readdir,
> fs/ocfs2/file.c: .iterate = ocfs2_readdir,
>
> maintainers cc'd
>
> fs/orangefs/dir.c: .iterate = orangefs_dir_iterate,
>
> New; maintainer cc'd
>
> fs/overlayfs/readdir.c: .iterate = ovl_iterate,
>
> Active maintainer, cc'd
>
> fs/proc/base.c: .iterate = proc_##LSM##_attr_dir_iterate, \
>
> Hmm. We need both SMACK and Apparmor to agree to this ... cc's added.
This is fine for AppArmor
>
> fs/vboxsf/dir.c: .iterate = vboxsf_dir_iterate,
>
> Also newly added. Maintainer cc'd.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-18 16:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <YvvBs+7YUcrzwV1a@ZenIV>
[not found] ` <CAHk-=wgkNwDikLfEkqLxCWR=pLi1rbPZ5eyE8FbfmXP2=r3qcw@mail.gmail.com>
2022-08-16 19:11 ` Switching to iterate_shared Matthew Wilcox
2022-08-16 22:30 ` Casey Schaufler
2022-08-16 23:09 ` Linus Torvalds
2022-08-18 14:35 ` Matthew Wilcox
2022-08-18 16:14 ` [apparmor] " John Johansen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).