| Message ID | 20231207123825.4011620-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Prepare for fsnotify pre-content permission events | expand |
On Thu, Dec 07, 2023 at 02:38:21PM +0200, Amir Goldstein wrote: > Hi Jan & Christian, > > I am not planning to post the fanotify pre-content event patches [1] > for 6.8. Not because they are not ready, but because the usersapce > example is not ready. > > Also, I think it is a good idea to let the large permission hooks > cleanup work to mature over the 6.8 cycle, before we introduce the > pre-content events. > > However, I would like to include the following vfs prep patches along > with the vfs.rw PR for 6.8, which could be titled as the subject of > this cover letter. > > Patch 1 is a variant of a cleanup suggested by Christoph to get rid > of the generic_copy_file_range() exported symbol. > > Patches 2,3 add the file_write_not_started() assertion to fsnotify > file permission hooks. IMO, it is important to merge it along with > vfs.rw because: > > 1. This assert is how I tested vfs.rw does what it aimed to achieve > 2. This will protect us from new callers that break the new order > 3. The commit message of patch 3 provides the context for the entire > series and can be included in the PR message > > Patch 4 is the final change of fsnotify permission hook locations/args > and is the last of the vfs prerequsites for pre-content events. > > If we merge patch 4 for 6.8, it will be much easier for the development > of fanotify pre-content events in 6.9 dev cycle, which be contained > within the fsnotify subsystem. Reviewed-by: Josef Bacik <josef@toxicpanda.com> Can you get an fstest added that exercises the freeze deadlock? I feel like we're going to break that at some point and I'd rather find out in testing than in production. Thanks, Josef
On Thu, Dec 7, 2023 at 11:51 PM Josef Bacik <josef@toxicpanda.com> wrote: > > On Thu, Dec 07, 2023 at 02:38:21PM +0200, Amir Goldstein wrote: > > Hi Jan & Christian, > > > > I am not planning to post the fanotify pre-content event patches [1] > > for 6.8. Not because they are not ready, but because the usersapce > > example is not ready. > > > > Also, I think it is a good idea to let the large permission hooks > > cleanup work to mature over the 6.8 cycle, before we introduce the > > pre-content events. > > > > However, I would like to include the following vfs prep patches along > > with the vfs.rw PR for 6.8, which could be titled as the subject of > > this cover letter. > > > > Patch 1 is a variant of a cleanup suggested by Christoph to get rid > > of the generic_copy_file_range() exported symbol. > > > > Patches 2,3 add the file_write_not_started() assertion to fsnotify > > file permission hooks. IMO, it is important to merge it along with > > vfs.rw because: > > > > 1. This assert is how I tested vfs.rw does what it aimed to achieve > > 2. This will protect us from new callers that break the new order > > 3. The commit message of patch 3 provides the context for the entire > > series and can be included in the PR message > > > > Patch 4 is the final change of fsnotify permission hook locations/args > > and is the last of the vfs prerequsites for pre-content events. > > > > If we merge patch 4 for 6.8, it will be much easier for the development > > of fanotify pre-content events in 6.9 dev cycle, which be contained > > within the fsnotify subsystem. > > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > > Can you get an fstest added that exercises the freeze deadlock? I suppose that you mean a test that exercises the lockdep assertion? This is much easier to do, so I don't see the point in actually testing the deadlock. The only thing is that the assertion will not be backported so this test would protect us from future regression, but will not nudge stable kernel users to backport the deadlock fix, which I don't think they should be doing anyway. It is actually already exercised by tests overlay/068,069, but I can add a generic test to get wider testing coverage. Thanks, Amir.
On Thu, 07 Dec 2023 14:38:21 +0200, Amir Goldstein wrote: > I am not planning to post the fanotify pre-content event patches [1] > for 6.8. Not because they are not ready, but because the usersapce > example is not ready. > > Also, I think it is a good idea to let the large permission hooks > cleanup work to mature over the 6.8 cycle, before we introduce the > pre-content events. > > [...] Picking this up to get it into -next rather sooner than later. But @Jan, I'll wait for your Acks. --- Applied to the vfs.rw branch of the vfs/vfs.git tree. Patches in the vfs.rw branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.rw [1/4] fs: use splice_copy_file_range() inline helper https://git.kernel.org/vfs/vfs/c/4955c918c9e5 [2/4] fsnotify: split fsnotify_perm() into two hooks https://git.kernel.org/vfs/vfs/c/d2fc40363ab1 [3/4] fsnotify: assert that file_start_write() is not held in permission hooks https://git.kernel.org/vfs/vfs/c/24065342b941 [4/4] fsnotify: pass access range in file permission hooks https://git.kernel.org/vfs/vfs/c/e5c56a33657b
On Fri, Dec 8, 2023 at 9:34 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Thu, Dec 7, 2023 at 11:51 PM Josef Bacik <josef@toxicpanda.com> wrote: > > > > On Thu, Dec 07, 2023 at 02:38:21PM +0200, Amir Goldstein wrote: > > > Hi Jan & Christian, > > > > > > I am not planning to post the fanotify pre-content event patches [1] > > > for 6.8. Not because they are not ready, but because the usersapce > > > example is not ready. > > > > > > Also, I think it is a good idea to let the large permission hooks > > > cleanup work to mature over the 6.8 cycle, before we introduce the > > > pre-content events. > > > > > > However, I would like to include the following vfs prep patches along > > > with the vfs.rw PR for 6.8, which could be titled as the subject of > > > this cover letter. > > > > > > Patch 1 is a variant of a cleanup suggested by Christoph to get rid > > > of the generic_copy_file_range() exported symbol. > > > > > > Patches 2,3 add the file_write_not_started() assertion to fsnotify > > > file permission hooks. IMO, it is important to merge it along with > > > vfs.rw because: > > > > > > 1. This assert is how I tested vfs.rw does what it aimed to achieve > > > 2. This will protect us from new callers that break the new order > > > 3. The commit message of patch 3 provides the context for the entire > > > series and can be included in the PR message > > > > > > Patch 4 is the final change of fsnotify permission hook locations/args > > > and is the last of the vfs prerequsites for pre-content events. > > > > > > If we merge patch 4 for 6.8, it will be much easier for the development > > > of fanotify pre-content events in 6.9 dev cycle, which be contained > > > within the fsnotify subsystem. > > > > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > > > > Can you get an fstest added that exercises the freeze deadlock? > > I suppose that you mean a test that exercises the lockdep assertion? > This is much easier to do, so I don't see the point in actually testing > the deadlock. The only thing is that the assertion will not be backported > so this test would protect us from future regression, but will not nudge > stable kernel users to backport the deadlock fix, which I don't think they > should be doing anyway. > > It is actually already exercised by tests overlay/068,069, but I can add > a generic test to get wider testing coverage. Here is a WIP test: https://github.com/amir73il/xfstests/commits/start-write-safe I tested it by reverting "fs: move file_start_write() into direct_splice_actor()" and seeing that it triggers the assert. Thanks, Amir.
On Fri, Dec 15, 2023 at 07:00:08PM +0200, Amir Goldstein wrote: > On Fri, Dec 8, 2023 at 9:34 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Thu, Dec 7, 2023 at 11:51 PM Josef Bacik <josef@toxicpanda.com> wrote: > > > > > > On Thu, Dec 07, 2023 at 02:38:21PM +0200, Amir Goldstein wrote: > > > > Hi Jan & Christian, > > > > > > > > I am not planning to post the fanotify pre-content event patches [1] > > > > for 6.8. Not because they are not ready, but because the usersapce > > > > example is not ready. > > > > > > > > Also, I think it is a good idea to let the large permission hooks > > > > cleanup work to mature over the 6.8 cycle, before we introduce the > > > > pre-content events. > > > > > > > > However, I would like to include the following vfs prep patches along > > > > with the vfs.rw PR for 6.8, which could be titled as the subject of > > > > this cover letter. > > > > > > > > Patch 1 is a variant of a cleanup suggested by Christoph to get rid > > > > of the generic_copy_file_range() exported symbol. > > > > > > > > Patches 2,3 add the file_write_not_started() assertion to fsnotify > > > > file permission hooks. IMO, it is important to merge it along with > > > > vfs.rw because: > > > > > > > > 1. This assert is how I tested vfs.rw does what it aimed to achieve > > > > 2. This will protect us from new callers that break the new order > > > > 3. The commit message of patch 3 provides the context for the entire > > > > series and can be included in the PR message > > > > > > > > Patch 4 is the final change of fsnotify permission hook locations/args > > > > and is the last of the vfs prerequsites for pre-content events. > > > > > > > > If we merge patch 4 for 6.8, it will be much easier for the development > > > > of fanotify pre-content events in 6.9 dev cycle, which be contained > > > > within the fsnotify subsystem. > > > > > > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > > > > > > Can you get an fstest added that exercises the freeze deadlock? > > > > I suppose that you mean a test that exercises the lockdep assertion? > > This is much easier to do, so I don't see the point in actually testing > > the deadlock. The only thing is that the assertion will not be backported > > so this test would protect us from future regression, but will not nudge > > stable kernel users to backport the deadlock fix, which I don't think they > > should be doing anyway. > > > > It is actually already exercised by tests overlay/068,069, but I can add > > a generic test to get wider testing coverage. > > Here is a WIP test: > https://github.com/amir73il/xfstests/commits/start-write-safe > > I tested it by reverting "fs: move file_start_write() into > direct_splice_actor()" > and seeing that it triggers the assert. Perfect, this is exactly the sort of thing I was hoping for. Thanks, Josef
Hi Jan & Christian, I am not planning to post the fanotify pre-content event patches [1] for 6.8. Not because they are not ready, but because the usersapce example is not ready. Also, I think it is a good idea to let the large permission hooks cleanup work to mature over the 6.8 cycle, before we introduce the pre-content events. However, I would like to include the following vfs prep patches along with the vfs.rw PR for 6.8, which could be titled as the subject of this cover letter. Patch 1 is a variant of a cleanup suggested by Christoph to get rid of the generic_copy_file_range() exported symbol. Patches 2,3 add the file_write_not_started() assertion to fsnotify file permission hooks. IMO, it is important to merge it along with vfs.rw because: 1. This assert is how I tested vfs.rw does what it aimed to achieve 2. This will protect us from new callers that break the new order 3. The commit message of patch 3 provides the context for the entire series and can be included in the PR message Patch 4 is the final change of fsnotify permission hook locations/args and is the last of the vfs prerequsites for pre-content events. If we merge patch 4 for 6.8, it will be much easier for the development of fanotify pre-content events in 6.9 dev cycle, which be contained within the fsnotify subsystem. Thanks, Amir. [1] https://github.com/amir73il/linux/commits/fan_pre_content Amir Goldstein (4): fs: use splice_copy_file_range() inline helper fsnotify: split fsnotify_perm() into two hooks fsnotify: assert that file_start_write() is not held in permission hooks fsnotify: pass access range in file permission hooks fs/ceph/file.c | 4 ++-- fs/fuse/file.c | 5 +++-- fs/nfs/nfs4file.c | 5 +++-- fs/open.c | 4 ++++ fs/read_write.c | 44 ++++++++-------------------------------- fs/readdir.c | 4 ++++ fs/remap_range.c | 8 +++++++- fs/smb/client/cifsfs.c | 5 +++-- fs/splice.c | 2 +- include/linux/fs.h | 3 --- include/linux/fsnotify.h | 42 ++++++++++++++++++++++++-------------- include/linux/splice.h | 8 ++++++++ security/security.c | 10 ++------- 13 files changed, 72 insertions(+), 72 deletions(-)