quota: check for register_sysctl() failure

[PATCH] quota: check for register_sysctl() failure

Posted by Yangtao Li 4 days, 5 hours ago

register_sysctl() might fail, call panic() as with other failure
checks in this function if register_sysctl() failed.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/quota/dquot.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 90cb70c82012..1483af271368 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2955,7 +2955,8 @@ static int __init dquot_init(void)
 
 	printk(KERN_NOTICE "VFS: Disk quotas %s\n", __DQUOT_VERSION__);
 
-	register_sysctl("fs/quota", fs_dqstats_table);
+	if (!register_sysctl("fs/quota", fs_dqstats_table))
+		panic("Cannot register dquot sysctl");
 
 	dquot_cachep = kmem_cache_create("dquot",
 			sizeof(struct dquot), sizeof(unsigned long) * 4,
-- 
2.35.1

Re: [PATCH] quota: check for register_sysctl() failure

Posted by Jan Kara 2 days, 10 hours ago

On Sun 19-03-23 00:06:39, Yangtao Li wrote:
> register_sysctl() might fail, call panic() as with other failure
> checks in this function if register_sysctl() failed.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>

...

> @@ -2955,7 +2955,8 @@ static int __init dquot_init(void)
>  
>  	printk(KERN_NOTICE "VFS: Disk quotas %s\n", __DQUOT_VERSION__);
>  
> -	register_sysctl("fs/quota", fs_dqstats_table);
> +	if (!register_sysctl("fs/quota", fs_dqstats_table))
> +		panic("Cannot register dquot sysctl");

Well, but this is going to make system unbootable with CONFIG_QUOTA &&
!CONFIG_SYSCTL. Quota functionality actually does not depend on sysctl
being available so just continuing without sysctl is perfectly fine.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] quota: check for register_sysctl() failure

Posted by Yangtao Li 2 days, 10 hours ago

> Well, but this is going to make system unbootable with
> CONFIG_QUOTA && !CONFIG_SYSCTL.
> Quota functionality actually does not depend on sysctl being
> available so just continuing without sysctl is perfectly fine.

To be honest, I'm a little confused. Should we panic if quota
registration fails, or should it depend on the user's cmdline
parameters? Just like in the filesystem where we can choose
whether to panic in case of exceptions.

Or, should we panic if registration fails when
CONFIG_QUOTA && CONFIG_SYSCTL are enabled?

BTW, kindly ping for:
https://lore.kernel.org/lkml/20230228103515.sb6qpvnmbvenvq73@quack3/

Not sure if you have forgotten to pick these two commits.

Thx,
Yangtao

Re: [PATCH] quota: check for register_sysctl() failure

Posted by Jan Kara 2 days, 4 hours ago

On Mon 20-03-23 19:40:28, Yangtao Li wrote:
> > Well, but this is going to make system unbootable with
> > CONFIG_QUOTA && !CONFIG_SYSCTL.
> > Quota functionality actually does not depend on sysctl being
> > available so just continuing without sysctl is perfectly fine.
> 
> To be honest, I'm a little confused. Should we panic if quota
> registration fails, or should it depend on the user's cmdline
> parameters? Just like in the filesystem where we can choose
> whether to panic in case of exceptions.
> 
> Or, should we panic if registration fails when
> CONFIG_QUOTA && CONFIG_SYSCTL are enabled?

So I think that just ignoring the failure to register sysctl (as is
currently happening) is fine. Honestly, in practice this is not a likely
scenario so I don't think it matters much but if we wanted, we could print
a message that sysctl registration failed if CONFIG_SYSCTL is enabled.
 
> BTW, kindly ping for:
> https://lore.kernel.org/lkml/20230228103515.sb6qpvnmbvenvq73@quack3/
> 
> Not sure if you have forgotten to pick these two commits.

Yeah, sorry. Now picked up and pushed.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR