linux-kernel - Re: [PATCH 2/2] staging: lustre: obdclass: Add handling of error returned by lustre_cfg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <97207CF4-21C1-4351-92BF-6F28E7281CF6@intel.com>
Date:   Sun, 6 Nov 2016 12:26:32 -0500
From:   Oleg Drokin <oleg.drokin@...el.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     <andreas.dilger@...el.com>, <jsimmons@...radead.org>,
        <gregkh@...uxfoundation.org>, <emoly.liu@...el.com>,
        <lustre-devel@...ts.lustre.org>, <devel@...verdev.osuosl.org>,
        <linux-kernel@...r.kernel.org>, <kernel-janitors@...r.kernel.org>
Subject: Re: [PATCH 2/2] staging: lustre: obdclass: Add handling of error returned by lustre_cfg_new

Hello!

On Nov 6, 2016, at 12:11 PM, Christophe JAILLET wrote:

> 'lustre_cfg_new()' can return ERR_PTR(-ENOMEM).
> Handle these errors and propagate the error code to the callers.
> 
> Error handling has been rearranged in 'lustre_process_log()' with the
> addition of a label in order to free some resources.

I wonder if we should just make it return NULL on allocation failure,
and then at least the other error handling that is there (i.e. in your other patch)
would become correct.
This would make handling in mgc_apply_recover_logs incorrect, but it's already
geared towards this sort of handling anyway, as it discards the passed error
and sets ENOMEM unconditionally (just need to revert 3092c34a in a way).

Thanks!

> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
> ---
> drivers/staging/lustre/lustre/obdclass/obd_mount.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/obdclass/obd_mount.c b/drivers/staging/lustre/lustre/obdclass/obd_mount.c
> index 59fbc29aae94..5473615cd338 100644
> --- a/drivers/staging/lustre/lustre/obdclass/obd_mount.c
> +++ b/drivers/staging/lustre/lustre/obdclass/obd_mount.c
> @@ -89,11 +89,14 @@ int lustre_process_log(struct super_block *sb, char *logname,
> 	lustre_cfg_bufs_set(bufs, 2, cfg, sizeof(*cfg));
> 	lustre_cfg_bufs_set(bufs, 3, &sb, sizeof(sb));
> 	lcfg = lustre_cfg_new(LCFG_LOG_START, bufs);
> +	if (IS_ERR(lcfg)) {
> +		rc = PTR_ERR(lcfg);
> +		goto out_free;
> +	}
> +
> 	rc = obd_process_config(mgc, sizeof(*lcfg), lcfg);
> 	lustre_cfg_free(lcfg);
> 
> -	kfree(bufs);
> -
> 	if (rc == -EINVAL)
> 		LCONSOLE_ERROR_MSG(0x15b, "%s: The configuration from log '%s' failed from the MGS (%d).  Make sure this client and the MGS are running compatible versions of Lustre.\n",
> 				   mgc->obd_name, logname, rc);
> @@ -104,6 +107,9 @@ int lustre_process_log(struct super_block *sb, char *logname,
> 				   rc);
> 
> 	/* class_obd_list(); */
> +
> +out_free:
> +	kfree(bufs);
> 	return rc;
> }
> EXPORT_SYMBOL(lustre_process_log);
> @@ -127,6 +133,9 @@ int lustre_end_log(struct super_block *sb, char *logname,
> 	if (cfg)
> 		lustre_cfg_bufs_set(&bufs, 2, cfg, sizeof(*cfg));
> 	lcfg = lustre_cfg_new(LCFG_LOG_END, &bufs);
> +	if (IS_ERR(lcfg))
> +		return PTR_ERR(lcfg);
> +
> 	rc = obd_process_config(mgc, sizeof(*lcfg), lcfg);
> 	lustre_cfg_free(lcfg);
> 	return rc;
> @@ -159,6 +168,9 @@ static int do_lcfg(char *cfgname, lnet_nid_t nid, int cmd,
> 		lustre_cfg_bufs_set_string(&bufs, 4, s4);
> 
> 	lcfg = lustre_cfg_new(cmd, &bufs);
> +	if (IS_ERR(lcfg))
> +		return PTR_ERR(lcfg);
> +
> 	lcfg->lcfg_nid = nid;
> 	rc = class_process_config(lcfg);
> 	lustre_cfg_free(lcfg);
> -- 
> 2.9.3