| Message ID | 20230831112431.2998368-1-bschubert@ddn.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Use exclusive lock for file_remove_privs | expand |
On Thu, Aug 31, 2023 at 01:24:29PM +0200, Bernd Schubert wrote: > While adding shared direct IO write locks to fuse Miklos noticed > that file_remove_privs() needs an exclusive lock. I then > noticed that btrfs actually has the same issue as I had in my patch, > it was calling into that function with a shared lock. > This series adds a new exported function file_needs_remove_privs(), > which used by the follow up btrfs patch and will be used by the > DIO code path in fuse as well. If that function returns any mask > the shared lock needs to be dropped and replaced by the exclusive > variant. > > Note: Compilation tested only. The fix makes sense, there should be no noticeable performance impact, basically the same check is done in the newly exported helper for the IS_NOSEC bit. I can give it a test locally for the default case, I'm not sure if we have specific tests for the security layers in fstests. Regarding merge, I can take the two patches via btrfs tree or can wait until the export is present in Linus' tree in case FUSE needs it independently.
On Tue, Sep 05, 2023 at 08:02:59PM +0200, David Sterba wrote: > On Thu, Aug 31, 2023 at 01:24:29PM +0200, Bernd Schubert wrote: > > While adding shared direct IO write locks to fuse Miklos noticed > > that file_remove_privs() needs an exclusive lock. I then > > noticed that btrfs actually has the same issue as I had in my patch, > > it was calling into that function with a shared lock. > > This series adds a new exported function file_needs_remove_privs(), > > which used by the follow up btrfs patch and will be used by the > > DIO code path in fuse as well. If that function returns any mask > > the shared lock needs to be dropped and replaced by the exclusive > > variant. > > > > Note: Compilation tested only. > > The fix makes sense, there should be no noticeable performance impact, > basically the same check is done in the newly exported helper for the > IS_NOSEC bit. I can give it a test locally for the default case, I'm > not sure if we have specific tests for the security layers in fstests. > > Regarding merge, I can take the two patches via btrfs tree or can wait > until the export is present in Linus' tree in case FUSE needs it > independently. Both fuse and btrfs need it afaict. We can grab it and provide a tag post -rc1? Whatever works best.
On 9/6/23 16:43, Christian Brauner wrote: > On Tue, Sep 05, 2023 at 08:02:59PM +0200, David Sterba wrote: >> On Thu, Aug 31, 2023 at 01:24:29PM +0200, Bernd Schubert wrote: >>> While adding shared direct IO write locks to fuse Miklos noticed >>> that file_remove_privs() needs an exclusive lock. I then >>> noticed that btrfs actually has the same issue as I had in my patch, >>> it was calling into that function with a shared lock. >>> This series adds a new exported function file_needs_remove_privs(), >>> which used by the follow up btrfs patch and will be used by the >>> DIO code path in fuse as well. If that function returns any mask >>> the shared lock needs to be dropped and replaced by the exclusive >>> variant. >>> >>> Note: Compilation tested only. >> >> The fix makes sense, there should be no noticeable performance impact, >> basically the same check is done in the newly exported helper for the >> IS_NOSEC bit. I can give it a test locally for the default case, I'm >> not sure if we have specific tests for the security layers in fstests. >> >> Regarding merge, I can take the two patches via btrfs tree or can wait >> until the export is present in Linus' tree in case FUSE needs it >> independently. > > Both fuse and btrfs need it afaict. We can grab it and provide a tag > post -rc1? Whatever works best. fuse will need it for my direct IO patches - hopefully in 6.7. For btrfs it is a bug fix, should go in asap? Christoph has some objections for to use the new exported helper (file_needs_remove_privs). Maybe I better send a version for btrfs that only uses S_NOSEC? For fuse we cannot use it, unfortunately. Thanks, Bernd
On Wed, Sep 06, 2023 at 04:51:20PM +0200, Bernd Schubert wrote: > > > On 9/6/23 16:43, Christian Brauner wrote: > > On Tue, Sep 05, 2023 at 08:02:59PM +0200, David Sterba wrote: > > > On Thu, Aug 31, 2023 at 01:24:29PM +0200, Bernd Schubert wrote: > > > > While adding shared direct IO write locks to fuse Miklos noticed > > > > that file_remove_privs() needs an exclusive lock. I then > > > > noticed that btrfs actually has the same issue as I had in my patch, > > > > it was calling into that function with a shared lock. > > > > This series adds a new exported function file_needs_remove_privs(), > > > > which used by the follow up btrfs patch and will be used by the > > > > DIO code path in fuse as well. If that function returns any mask > > > > the shared lock needs to be dropped and replaced by the exclusive > > > > variant. > > > > > > > > Note: Compilation tested only. > > > > > > The fix makes sense, there should be no noticeable performance impact, > > > basically the same check is done in the newly exported helper for the > > > IS_NOSEC bit. I can give it a test locally for the default case, I'm > > > not sure if we have specific tests for the security layers in fstests. > > > > > > Regarding merge, I can take the two patches via btrfs tree or can wait > > > until the export is present in Linus' tree in case FUSE needs it > > > independently. > > > > Both fuse and btrfs need it afaict. We can grab it and provide a tag > > post -rc1? Whatever works best. > > fuse will need it for my direct IO patches - hopefully in 6.7. > For btrfs it is a bug fix, should go in asap? > > Christoph has some objections for to use the new exported helper > (file_needs_remove_privs). Maybe I better send a version for btrfs > that only uses S_NOSEC? For fuse we cannot use it, unfortunately. Sure.
On Wed, Sep 06, 2023 at 04:43:22PM +0200, Christian Brauner wrote: > On Tue, Sep 05, 2023 at 08:02:59PM +0200, David Sterba wrote: > > On Thu, Aug 31, 2023 at 01:24:29PM +0200, Bernd Schubert wrote: > > > While adding shared direct IO write locks to fuse Miklos noticed > > > that file_remove_privs() needs an exclusive lock. I then > > > noticed that btrfs actually has the same issue as I had in my patch, > > > it was calling into that function with a shared lock. > > > This series adds a new exported function file_needs_remove_privs(), > > > which used by the follow up btrfs patch and will be used by the > > > DIO code path in fuse as well. If that function returns any mask > > > the shared lock needs to be dropped and replaced by the exclusive > > > variant. > > > > > > Note: Compilation tested only. > > > > The fix makes sense, there should be no noticeable performance impact, > > basically the same check is done in the newly exported helper for the > > IS_NOSEC bit. I can give it a test locally for the default case, I'm > > not sure if we have specific tests for the security layers in fstests. > > > > Regarding merge, I can take the two patches via btrfs tree or can wait > > until the export is present in Linus' tree in case FUSE needs it > > independently. > > Both fuse and btrfs need it afaict. We can grab it and provide a tag > post -rc1? Whatever works best. Git tree sync won't be needed, Bernd sent the fix within btrfs code.