| Message ID | 20240730085856.32385-1-olaf@aepfle.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] mount: handle OOM on mnt_warn_timestamp_expiry | expand |
On Tue, Jul 30, 2024 at 10:58:13AM GMT, Olaf Hering wrote: > If no page could be allocated, an error pointer was used as format > string in pr_warn. > > Rearrange the code to return early in case of OOM. Also add a check > for the return value of d_path. The API of that function is not > documented. It currently returns only ERR_PTR values, but may return > also NULL in the future. Use PTR_ERR_OR_ZERO to cover both cases. > > Fixes: f8b92ba67c5d ("mount: Add mount warning for impending timestamp expiry") > > Signed-off-by: Olaf Hering <olaf@aepfle.de> > --- > fs/namespace.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/fs/namespace.c b/fs/namespace.c > index 328087a4df8a..539d4f203a20 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -2922,7 +2922,14 @@ static void mnt_warn_timestamp_expiry(struct path *mountpoint, struct vfsmount * > (!(sb->s_iflags & SB_I_TS_EXPIRY_WARNED)) && > (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); > + char *mntpath; > + > + if (!buf) > + return; > + > + mntpath = d_path(mountpoint, buf, PAGE_SIZE); > + if (PTR_ERR_OR_ZERO(mntpath)) This needs to be IS_ERR_OR_NULL(). > + goto err; We should still warn when decoding the mountpoint fails. I'll just amend your patch to something like: diff --git a/fs/namespace.c b/fs/namespace.c index 328087a4df8a..0f2f140aaf05 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -2921,16 +2921,21 @@ static void mnt_warn_timestamp_expiry(struct path *mountpoint, struct vfsmount * if (!__mnt_is_readonly(mnt) && (!(sb->s_iflags & SB_I_TS_EXPIRY_WARNED)) && (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); + char *buf, *mntpath = NULL; + + buf = (char *)__get_free_page(GFP_KERNEL); + if (buf) + mntpath = d_path(mountpoint, buf, PAGE_SIZE); + if (IS_ERR_OR_NULL(mntpath)) + mntpath = "(unknown)"; pr_warn("%s filesystem being %s at %s supports timestamps until %ptTd (0x%llx)\n", sb->s_type->name, is_mounted(mnt) ? "remounted" : "mounted", mntpath, &sb->s_time_max, (unsigned long long)sb->s_time_max); - - free_page((unsigned long)buf); + if (buf) + free_page((unsigned long)buf); sb->s_iflags |= SB_I_TS_EXPIRY_WARNED; } }
On Tue 30-07-24 11:49:37, Christian Brauner wrote: > On Tue, Jul 30, 2024 at 10:58:13AM GMT, Olaf Hering wrote: > > If no page could be allocated, an error pointer was used as format > > string in pr_warn. > > > > Rearrange the code to return early in case of OOM. Also add a check > > for the return value of d_path. The API of that function is not > > documented. It currently returns only ERR_PTR values, but may return > > also NULL in the future. Use PTR_ERR_OR_ZERO to cover both cases. > > > > Fixes: f8b92ba67c5d ("mount: Add mount warning for impending timestamp expiry") > > > > Signed-off-by: Olaf Hering <olaf@aepfle.de> > > --- > > fs/namespace.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/fs/namespace.c b/fs/namespace.c > > index 328087a4df8a..539d4f203a20 100644 > > --- a/fs/namespace.c > > +++ b/fs/namespace.c > > @@ -2922,7 +2922,14 @@ static void mnt_warn_timestamp_expiry(struct path *mountpoint, struct vfsmount * > > (!(sb->s_iflags & SB_I_TS_EXPIRY_WARNED)) && > > (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); > > + char *mntpath; > > + > > + if (!buf) > > + return; > > + > > + mntpath = d_path(mountpoint, buf, PAGE_SIZE); > > + if (PTR_ERR_OR_ZERO(mntpath)) > > This needs to be IS_ERR_OR_NULL(). > > > + goto err; > > We should still warn when decoding the mountpoint fails. I'll just amend > your patch to something like: Looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > > diff --git a/fs/namespace.c b/fs/namespace.c > index 328087a4df8a..0f2f140aaf05 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -2921,16 +2921,21 @@ static void mnt_warn_timestamp_expiry(struct path *mountpoint, struct vfsmount * > if (!__mnt_is_readonly(mnt) && > (!(sb->s_iflags & SB_I_TS_EXPIRY_WARNED)) && > (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); > + char *buf, *mntpath = NULL; > + > + buf = (char *)__get_free_page(GFP_KERNEL); > + if (buf) > + mntpath = d_path(mountpoint, buf, PAGE_SIZE); > + if (IS_ERR_OR_NULL(mntpath)) > + mntpath = "(unknown)"; > > pr_warn("%s filesystem being %s at %s supports timestamps until %ptTd (0x%llx)\n", > sb->s_type->name, > is_mounted(mnt) ? "remounted" : "mounted", > mntpath, &sb->s_time_max, > (unsigned long long)sb->s_time_max); > - > - free_page((unsigned long)buf); > + if (buf) > + free_page((unsigned long)buf); > sb->s_iflags |= SB_I_TS_EXPIRY_WARNED; > } > } >
On Tue, Jul 30, 2024 at 10:58:13AM +0200, Olaf Hering wrote: > If no page could be allocated, an error pointer was used as format > string in pr_warn. > > Rearrange the code to return early in case of OOM. Also add a check > for the return value of d_path. The API of that function is not > documented. It currently returns only ERR_PTR values, but may return > also NULL in the future. Use PTR_ERR_OR_ZERO to cover both cases. Don't use PTR_ERR_OR_ZERO. And don't mix ERR_PTR() and NULL for error returns without a really good reason for that. d_path() is *NOT* going to return NULL. NAK in that form.
Tue, 30 Jul 2024 16:49:24 +0100 Al Viro <viro@zeniv.linux.org.uk>:
> d_path() is *NOT* going to return NULL.
The existing documentation does not state that fact.
Olaf
On Tue, Jul 30, 2024 at 09:58:27PM +0200, Olaf Hering wrote: > Tue, 30 Jul 2024 16:49:24 +0100 Al Viro <viro@zeniv.linux.org.uk>: > > > d_path() is *NOT* going to return NULL. > > The existing documentation does not state that fact. Needs to be fixed, but as a general rule - mixing NULL and ERR_PTR() for error reporting is a Very Bad Idea(tm). There are cases when there's a legitimate reason for a function to return both, but they are rare and NULL should not be an error case. Example: d_splice_alias(); ERR_PTR(-E...) => error; NULL => success, passed candidate had been accepted and attached to inode; pointer to struct dentry instance => success, preexisting alias returned and should be used instead of the candidate. Using IS_ERR_OR_NULL for "future-proofing" is obfuscating the things for no good reason - it confuses the readers, and it tends to spread when people are copying the code around. Please, don't do it.
diff --git a/fs/namespace.c b/fs/namespace.c index 328087a4df8a..539d4f203a20 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -2922,7 +2922,14 @@ static void mnt_warn_timestamp_expiry(struct path *mountpoint, struct vfsmount * (!(sb->s_iflags & SB_I_TS_EXPIRY_WARNED)) && (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); + char *mntpath; + + if (!buf) + return; + + mntpath = d_path(mountpoint, buf, PAGE_SIZE); + if (PTR_ERR_OR_ZERO(mntpath)) + goto err; pr_warn("%s filesystem being %s at %s supports timestamps until %ptTd (0x%llx)\n", sb->s_type->name, @@ -2930,8 +2937,9 @@ static void mnt_warn_timestamp_expiry(struct path *mountpoint, struct vfsmount * mntpath, &sb->s_time_max, (unsigned long long)sb->s_time_max); - free_page((unsigned long)buf); sb->s_iflags |= SB_I_TS_EXPIRY_WARNED; +err: + free_page((unsigned long)buf); } }
If no page could be allocated, an error pointer was used as format string in pr_warn. Rearrange the code to return early in case of OOM. Also add a check for the return value of d_path. The API of that function is not documented. It currently returns only ERR_PTR values, but may return also NULL in the future. Use PTR_ERR_OR_ZERO to cover both cases. Fixes: f8b92ba67c5d ("mount: Add mount warning for impending timestamp expiry") Signed-off-by: Olaf Hering <olaf@aepfle.de> --- fs/namespace.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)