| Message ID | ea6a2384807500090943f95c164e9f6b899efc58.1710246349.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: validate device maj:min during scan | expand |
On Tue, Mar 12, 2024 at 06:32:41PM +0530, Anand Jain wrote: > The maj:min of a device can change without altering the device path. > When the device is re-scanned, only the device path change is fixed, > if any, but the changed maj:min remains (bug). This patch fixes it by > also checking for the changed maj:min. > > However, please note that we still need to validate the maj:min during > open as in the patch ("btrfs: validate device maj:min during open") because > only the device specified in the mount command gets scanned during mount. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> Is this a real problem you can reproduce? I'm pretty sure we can't hit this code path with single dev fs due to the temp_fsid logic. But it does seem plausible to hit it with a multi device fs. If you can in fact reproduce it, please feel free to add: Reviewed-by: Boris Burkov <boris@bur.io> and please also send an fstests patch with the reproducer! > --- > fs/btrfs/volumes.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 8a35605822bf..473f03965f26 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -854,7 +854,8 @@ static noinline struct btrfs_device *device_list_add(const char *path, > MAJOR(path_devt), MINOR(path_devt), > current->comm, task_pid_nr(current)); > > - } else if (!device->name || strcmp(device->name->str, path)) { > + } else if (!device->name || strcmp(device->name->str, path) || > + device->devt != path_devt) { > /* > * When FS is already mounted. > * 1. If you are here and if the device->name is NULL that > -- > 2.31.1 >
On 3/13/24 00:44, Boris Burkov wrote: > On Tue, Mar 12, 2024 at 06:32:41PM +0530, Anand Jain wrote: >> The maj:min of a device can change without altering the device path. >> When the device is re-scanned, only the device path change is fixed, >> if any, but the changed maj:min remains (bug). This patch fixes it by >> also checking for the changed maj:min. >> >> However, please note that we still need to validate the maj:min during >> open as in the patch ("btrfs: validate device maj:min during open") because >> only the device specified in the mount command gets scanned during mount. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> > > Is this a real problem you can reproduce? I'm pretty sure we can't hit > this code path with single dev fs due to the temp_fsid logic. But it > does seem plausible to hit it with a multi device fs. > > If you can in fact reproduce it, please feel free to add: > > Reviewed-by: Boris Burkov <boris@bur.io> > > and please also send an fstests patch with the reproducer! Hm. It is only theoretical. I do not have a test case because I am assuming the patch ("btrfs: validate device maj:min during open") is integrated, which means any stale devt gets fixed during mount. So it is hard to break. If you have any ideas for a test case, please share. Thanks, Anand >> --- >> fs/btrfs/volumes.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 8a35605822bf..473f03965f26 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -854,7 +854,8 @@ static noinline struct btrfs_device *device_list_add(const char *path, >> MAJOR(path_devt), MINOR(path_devt), >> current->comm, task_pid_nr(current)); >> >> - } else if (!device->name || strcmp(device->name->str, path)) { >> + } else if (!device->name || strcmp(device->name->str, path) || >> + device->devt != path_devt) { >> /* >> * When FS is already mounted. >> * 1. If you are here and if the device->name is NULL that >> -- >> 2.31.1 >>
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 8a35605822bf..473f03965f26 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -854,7 +854,8 @@ static noinline struct btrfs_device *device_list_add(const char *path, MAJOR(path_devt), MINOR(path_devt), current->comm, task_pid_nr(current)); - } else if (!device->name || strcmp(device->name->str, path)) { + } else if (!device->name || strcmp(device->name->str, path) || + device->devt != path_devt) { /* * When FS is already mounted. * 1. If you are here and if the device->name is NULL that
The maj:min of a device can change without altering the device path. When the device is re-scanned, only the device path change is fixed, if any, but the changed maj:min remains (bug). This patch fixes it by also checking for the changed maj:min. However, please note that we still need to validate the maj:min during open as in the patch ("btrfs: validate device maj:min during open") because only the device specified in the mount command gets scanned during mount. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/volumes.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)