| Message ID | 20190730014924.2193-5-deepa.kernel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs: Add support for timestamp limits | expand |
On Mon, 2019-07-29 at 18:49 -0700, Deepa Dinamani wrote: > The warning reuses the uptime max of 30 years used by the > setitimeofday(). > > Note that the warning is only added for new filesystem mounts > through the mount syscall. Automounts do not have the same warning. > > Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com> > --- > fs/namespace.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/fs/namespace.c b/fs/namespace.c > index b26778bdc236..5314fac8035e 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -2739,6 +2739,17 @@ static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint, > error = do_add_mount(real_mount(mnt), mountpoint, mnt_flags); > if (error < 0) > mntput(mnt); > + > + if (!error && sb->s_time_max && I don't know why you are testing sb->s_time_max here - it should always be non-zero since alloc_super() sets it to TIME64_MAX. > + (ktime_get_real_seconds() + TIME_UPTIME_SEC_MAX > sb->s_time_max)) { > + char *buf = (char *)__get_free_page(GFP_KERNEL); > + char *mntpath = buf ? d_path(mountpoint, buf, PAGE_SIZE) : ERR_PTR(-ENOMEM); > + > + pr_warn("Mounted %s file system at %s supports timestamps until 0x%llx\n", > + fc->fs_type->name, mntpath, (unsigned long long)sb->s_time_max); This doesn't seem like a helpful way to log the time. Maybe use time64_to_tm() to convert to "broken down" time and then print it with "%ptR"... but that wants struct rtc_time. If you apply the attached patch, however, you should then be able to print struct tm with "%ptT". Ben. > + free_page((unsigned long)buf); > + } > + > return error; > } >
On Mon, 2019-07-29 at 18:49 -0700, Deepa Dinamani wrote: > The warning reuses the uptime max of 30 years used by the > setitimeofday(). > > Note that the warning is only added for new filesystem mounts > through the mount syscall. Automounts do not have the same warning. [...] Another thing - perhaps this warning should be suppressed for read-only mounts? Ben.
On Mon, Aug 5, 2019 at 4:12 PM Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > > On Mon, 2019-07-29 at 18:49 -0700, Deepa Dinamani wrote: > > The warning reuses the uptime max of 30 years used by the > > setitimeofday(). > > > > Note that the warning is only added for new filesystem mounts > > through the mount syscall. Automounts do not have the same warning. > > > > Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com> > > --- > > fs/namespace.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/fs/namespace.c b/fs/namespace.c > > index b26778bdc236..5314fac8035e 100644 > > --- a/fs/namespace.c > > +++ b/fs/namespace.c > > @@ -2739,6 +2739,17 @@ static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint, > > error = do_add_mount(real_mount(mnt), mountpoint, mnt_flags); > > if (error < 0) > > mntput(mnt); > > + > > + if (!error && sb->s_time_max && > > I don't know why you are testing sb->s_time_max here - it should always > be non-zero since alloc_super() sets it to TIME64_MAX. I think we support some writable file systems that have no timestamps at all, so both the minimum and maximum default to 0 (1970-01-01). For these, there is no point in printing a warning, they just work as designed, even though the maximum is expired. Arnd
On Mon, Aug 5, 2019 at 7:14 AM Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > > On Mon, 2019-07-29 at 18:49 -0700, Deepa Dinamani wrote: > > The warning reuses the uptime max of 30 years used by the > > setitimeofday(). > > > > Note that the warning is only added for new filesystem mounts > > through the mount syscall. Automounts do not have the same warning. > [...] > > Another thing - perhaps this warning should be suppressed for read-only > mounts? Many filesystems support read only mounts only. We do fill in right granularities and limits for these filesystems as well. In keeping with the trend, I have added the warning accordingly. I don't think I have a preference either way. But, not warning for the red only mounts adds another if case. If you have a strong preference, I could add it in. -Deepa
> This doesn't seem like a helpful way to log the time. Maybe use > time64_to_tm() to convert to "broken down" time and then print it with > "%ptR"... but that wants struct rtc_time. If you apply the attached > patch, however, you should then be able to print struct tm with > "%ptT". OK. Will print a more user friendly date string here. Thanks, -Deepa
On Sat, 2019-08-10 at 13:44 -0700, Deepa Dinamani wrote: > On Mon, Aug 5, 2019 at 7:14 AM Ben Hutchings > <ben.hutchings@codethink.co.uk> wrote: > > On Mon, 2019-07-29 at 18:49 -0700, Deepa Dinamani wrote: > > > The warning reuses the uptime max of 30 years used by the > > > setitimeofday(). > > > > > > Note that the warning is only added for new filesystem mounts > > > through the mount syscall. Automounts do not have the same warning. > > [...] > > > > Another thing - perhaps this warning should be suppressed for read-only > > mounts? > > Many filesystems support read only mounts only. We do fill in right > granularities and limits for these filesystems as well. In keeping > with the trend, I have added the warning accordingly. I don't think I > have a preference either way. But, not warning for the red only mounts > adds another if case. If you have a strong preference, I could add it > in. It seems to me that the warning is needed if there is a possibility of data loss (incorrect timestamps, potentially leading to incorrect decisions about which files are newer). This can happen only when a filesystem is mounted read-write, or when a filesystem image is created. I think that warning for read-only mounts would be an annoyance to users retrieving files from old filesystems. Ben.
On Mon, Aug 12, 2019 at 3:25 PM Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > On Sat, 2019-08-10 at 13:44 -0700, Deepa Dinamani wrote: > > On Mon, Aug 5, 2019 at 7:14 AM Ben Hutchings > > <ben.hutchings@codethink.co.uk> wrote: > > > On Mon, 2019-07-29 at 18:49 -0700, Deepa Dinamani wrote: > > > > The warning reuses the uptime max of 30 years used by the > > > > setitimeofday(). > > > > > > > > Note that the warning is only added for new filesystem mounts > > > > through the mount syscall. Automounts do not have the same warning. > > > [...] > > > > > > Another thing - perhaps this warning should be suppressed for read-only > > > mounts? > > > > Many filesystems support read only mounts only. We do fill in right > > granularities and limits for these filesystems as well. In keeping > > with the trend, I have added the warning accordingly. I don't think I > > have a preference either way. But, not warning for the red only mounts > > adds another if case. If you have a strong preference, I could add it > > in. > > It seems to me that the warning is needed if there is a possibility of > data loss (incorrect timestamps, potentially leading to incorrect > decisions about which files are newer). This can happen only when a > filesystem is mounted read-write, or when a filesystem image is > created. > > I think that warning for read-only mounts would be an annoyance to > users retrieving files from old filesystems. I agree, the warning is not helpful for read-only mounts. An earlier plan was to completely disallow writable mounts that might risk an overflow (in some configurations at least). The warning replaces that now, and I think it should also just warn for the cases that would otherwise have been dangerous. Arnd
On Mon, Aug 12, 2019 at 7:11 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Mon, Aug 12, 2019 at 3:25 PM Ben Hutchings > <ben.hutchings@codethink.co.uk> wrote: > > On Sat, 2019-08-10 at 13:44 -0700, Deepa Dinamani wrote: > > > On Mon, Aug 5, 2019 at 7:14 AM Ben Hutchings > > > <ben.hutchings@codethink.co.uk> wrote: > > > > On Mon, 2019-07-29 at 18:49 -0700, Deepa Dinamani wrote: > > > > > The warning reuses the uptime max of 30 years used by the > > > > > setitimeofday(). > > > > > > > > > > Note that the warning is only added for new filesystem mounts > > > > > through the mount syscall. Automounts do not have the same warning. > > > > [...] > > > > > > > > Another thing - perhaps this warning should be suppressed for read-only > > > > mounts? > > > > > > Many filesystems support read only mounts only. We do fill in right > > > granularities and limits for these filesystems as well. In keeping > > > with the trend, I have added the warning accordingly. I don't think I > > > have a preference either way. But, not warning for the red only mounts > > > adds another if case. If you have a strong preference, I could add it > > > in. > > > > It seems to me that the warning is needed if there is a possibility of > > data loss (incorrect timestamps, potentially leading to incorrect > > decisions about which files are newer). This can happen only when a > > filesystem is mounted read-write, or when a filesystem image is > > created. > > > > I think that warning for read-only mounts would be an annoyance to > > users retrieving files from old filesystems. > > I agree, the warning is not helpful for read-only mounts. An earlier > plan was to completely disallow writable mounts that might risk an > overflow (in some configurations at least). The warning replaces that > now, and I think it should also just warn for the cases that would > otherwise have been dangerous. Ok, I will make the change to exclude new read only mounts. I will use __mnt_is_readonly() so that it also exculdes filesystems that are readonly also. The diff looks like below: - if (!error && sb->s_time_max && + if (!error && !__mnt_is_readonly(mnt) && (ktime_get_real_seconds() + TIME_UPTIME_SEC_MAX > sb->s_time_max)) { Note that we can get rid of checking for non zero sb->s_time_max now. -Deepa
On Mon, Aug 12, 2019 at 9:09 AM Deepa Dinamani <deepa.kernel@gmail.com> wrote: > > On Mon, Aug 12, 2019 at 7:11 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Mon, Aug 12, 2019 at 3:25 PM Ben Hutchings > > <ben.hutchings@codethink.co.uk> wrote: > > > On Sat, 2019-08-10 at 13:44 -0700, Deepa Dinamani wrote: > > > > On Mon, Aug 5, 2019 at 7:14 AM Ben Hutchings > > > > <ben.hutchings@codethink.co.uk> wrote: > > > > > On Mon, 2019-07-29 at 18:49 -0700, Deepa Dinamani wrote: > > > > > > The warning reuses the uptime max of 30 years used by the > > > > > > setitimeofday(). > > > > > > > > > > > > Note that the warning is only added for new filesystem mounts > > > > > > through the mount syscall. Automounts do not have the same warning. > > > > > [...] > > > > > > > > > > Another thing - perhaps this warning should be suppressed for read-only > > > > > mounts? > > > > > > > > Many filesystems support read only mounts only. We do fill in right > > > > granularities and limits for these filesystems as well. In keeping > > > > with the trend, I have added the warning accordingly. I don't think I > > > > have a preference either way. But, not warning for the red only mounts > > > > adds another if case. If you have a strong preference, I could add it > > > > in. > > > > > > It seems to me that the warning is needed if there is a possibility of > > > data loss (incorrect timestamps, potentially leading to incorrect > > > decisions about which files are newer). This can happen only when a > > > filesystem is mounted read-write, or when a filesystem image is > > > created. > > > > > > I think that warning for read-only mounts would be an annoyance to > > > users retrieving files from old filesystems. > > > > I agree, the warning is not helpful for read-only mounts. An earlier > > plan was to completely disallow writable mounts that might risk an > > overflow (in some configurations at least). The warning replaces that > > now, and I think it should also just warn for the cases that would > > otherwise have been dangerous. > > Ok, I will make the change to exclude new read only mounts. I will use > __mnt_is_readonly() so that it also exculdes filesystems that are > readonly also. > The diff looks like below: > > - if (!error && sb->s_time_max && > + if (!error && !__mnt_is_readonly(mnt) && > (ktime_get_real_seconds() + TIME_UPTIME_SEC_MAX > sb->s_time_max)) { > > Note that we can get rid of checking for non zero sb->s_time_max now. One more thing, we will probably have to add a second warning for when the filesystem is re-mounted rw after the initial readonly mount. -Deepa
On Mon, 2019-08-12 at 09:15 -0700, Deepa Dinamani wrote: > On Mon, Aug 12, 2019 at 9:09 AM Deepa Dinamani <deepa.kernel@gmail.com> wrote: > > On Mon, Aug 12, 2019 at 7:11 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > On Mon, Aug 12, 2019 at 3:25 PM Ben Hutchings > > > <ben.hutchings@codethink.co.uk> wrote: > > > > On Sat, 2019-08-10 at 13:44 -0700, Deepa Dinamani wrote: > > > > > On Mon, Aug 5, 2019 at 7:14 AM Ben Hutchings > > > > > <ben.hutchings@codethink.co.uk> wrote: > > > > > > On Mon, 2019-07-29 at 18:49 -0700, Deepa Dinamani wrote: > > > > > > > The warning reuses the uptime max of 30 years used by the > > > > > > > setitimeofday(). > > > > > > > > > > > > > > Note that the warning is only added for new filesystem mounts > > > > > > > through the mount syscall. Automounts do not have the same warning. > > > > > > [...] > > > > > > > > > > > > Another thing - perhaps this warning should be suppressed for read-only > > > > > > mounts? > > > > > > > > > > Many filesystems support read only mounts only. We do fill in right > > > > > granularities and limits for these filesystems as well. In keeping > > > > > with the trend, I have added the warning accordingly. I don't think I > > > > > have a preference either way. But, not warning for the red only mounts > > > > > adds another if case. If you have a strong preference, I could add it > > > > > in. > > > > > > > > It seems to me that the warning is needed if there is a possibility of > > > > data loss (incorrect timestamps, potentially leading to incorrect > > > > decisions about which files are newer). This can happen only when a > > > > filesystem is mounted read-write, or when a filesystem image is > > > > created. > > > > > > > > I think that warning for read-only mounts would be an annoyance to > > > > users retrieving files from old filesystems. > > > > > > I agree, the warning is not helpful for read-only mounts. An earlier > > > plan was to completely disallow writable mounts that might risk an > > > overflow (in some configurations at least). The warning replaces that > > > now, and I think it should also just warn for the cases that would > > > otherwise have been dangerous. > > > > Ok, I will make the change to exclude new read only mounts. I will use > > __mnt_is_readonly() so that it also exculdes filesystems that are > > readonly also. > > The diff looks like below: > > > > - if (!error && sb->s_time_max && > > + if (!error && !__mnt_is_readonly(mnt) && > > (ktime_get_real_seconds() + TIME_UPTIME_SEC_MAX > sb->s_time_max)) { > > > > Note that we can get rid of checking for non zero sb->s_time_max now. > > One more thing, we will probably have to add a second warning for when > the filesystem is re-mounted rw after the initial readonly mount. Indeed, there would need to be a check for remount-read-write. I didn't check whether remount uses this same code path. Ben.
diff --git a/fs/namespace.c b/fs/namespace.c index b26778bdc236..5314fac8035e 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -2739,6 +2739,17 @@ static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint, error = do_add_mount(real_mount(mnt), mountpoint, mnt_flags); if (error < 0) mntput(mnt); + + if (!error && sb->s_time_max && + (ktime_get_real_seconds() + TIME_UPTIME_SEC_MAX > sb->s_time_max)) { + char *buf = (char *)__get_free_page(GFP_KERNEL); + char *mntpath = buf ? d_path(mountpoint, buf, PAGE_SIZE) : ERR_PTR(-ENOMEM); + + pr_warn("Mounted %s file system at %s supports timestamps until 0x%llx\n", + fc->fs_type->name, mntpath, (unsigned long long)sb->s_time_max); + free_page((unsigned long)buf); + } + return error; }
The warning reuses the uptime max of 30 years used by the setitimeofday(). Note that the warning is only added for new filesystem mounts through the mount syscall. Automounts do not have the same warning. Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com> --- fs/namespace.c | 11 +++++++++++ 1 file changed, 11 insertions(+)