Patchwork cifs: fix error handling in mount-time DFS referral chasing code

login
register
mail settings
Submitter Jeff Layton
Date July 23, 2009, 7:22 p.m.
Message ID <1248376950-12366-1-git-send-email-jlayton@redhat.com>
Download mbox | patch
Permalink /patch/30149/
State New
Headers show

Comments

Jeff Layton - July 23, 2009, 7:22 p.m.
If the referral is malformed or the hostname can't be resolved, then
the current code generates an oops. Fix it to handle these errors
gracefully.

Reported-by: Sandro Mathys <sm@sandro-mathys.ch>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifs_dfs_ref.c |   12 +++++++++---
 fs/cifs/connect.c      |   13 ++++++++++---
 2 files changed, 19 insertions(+), 6 deletions(-)
Igor Mammedov - July 27, 2009, 9:44 a.m.
Looks good to me:

Acked-by: Igor Mammedov <niallain@gmail.com>

Jeff Layton wrote:
> If the referral is malformed or the hostname can't be resolved, then
> the current code generates an oops. Fix it to handle these errors
> gracefully.
> 
> Reported-by: Sandro Mathys <sm@sandro-mathys.ch>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/cifs_dfs_ref.c |   12 +++++++++---
>  fs/cifs/connect.c      |   13 ++++++++++---
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
> index 3bb11be..606912d 100644
> --- a/fs/cifs/cifs_dfs_ref.c
> +++ b/fs/cifs/cifs_dfs_ref.c
> @@ -55,7 +55,7 @@ void cifs_dfs_release_automount_timer(void)
>   * i.e. strips from UNC trailing path that is not part of share
>   * name and fixup missing '\' in the begining of DFS node refferal
>   * if neccessary.
> - * Returns pointer to share name on success or NULL on error.
> + * Returns pointer to share name on success or ERR_PTR on error.
>   * Caller is responsible for freeing returned string.
>   */
>  static char *cifs_get_share_name(const char *node_name)
> @@ -68,7 +68,7 @@ static char *cifs_get_share_name(const char *node_name)
>  	UNC = kmalloc(len+2 /*for term null and additional \ if it's missed */,
>  			 GFP_KERNEL);
>  	if (!UNC)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	/* get share name and server name */
>  	if (node_name[1] != '\\') {
> @@ -87,7 +87,7 @@ static char *cifs_get_share_name(const char *node_name)
>  		cERROR(1, ("%s: no server name end in node name: %s",
>  			__func__, node_name));
>  		kfree(UNC);
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	/* find sharename end */
> @@ -133,6 +133,12 @@ char *cifs_compose_mount_options(const char *sb_mountdata,
>  		return ERR_PTR(-EINVAL);
>  
>  	*devname = cifs_get_share_name(ref->node_name);
> +	if (IS_ERR(*devname)) {
> +		rc = PTR_ERR(*devname);
> +		*devname = NULL;
> +		goto compose_mount_options_err;
> +	}
> +
>  	rc = dns_resolve_server_name_to_ip(*devname, &srvIP);
>  	if (rc != 0) {
>  		cERROR(1, ("%s: Failed to resolve server part of %s to IP: %d",
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index fc44d31..f248688 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2544,11 +2544,20 @@ remote_path_check:
>  
>  			if (mount_data != mount_data_global)
>  				kfree(mount_data);
> +
>  			mount_data = cifs_compose_mount_options(
>  					cifs_sb->mountdata, full_path + 1,
>  					referrals, &fake_devname);
> -			kfree(fake_devname);
> +
>  			free_dfs_info_array(referrals, num_referrals);
> +			kfree(fake_devname);
> +			kfree(full_path);
> +
> +			if (IS_ERR(mount_data)) {
> +				rc = PTR_ERR(mount_data);
> +				mount_data = NULL;
> +				goto mount_fail_check;
> +			}
>  
>  			if (tcon)
>  				cifs_put_tcon(tcon);
> @@ -2556,8 +2565,6 @@ remote_path_check:
>  				cifs_put_smb_ses(pSesInfo);
>  
>  			cleanup_volume_info(&volume_info);
> -			FreeXid(xid);
> -			kfree(full_path);
>  			referral_walks_count++;
>  			goto try_mount_again;
>  		}
Steve French - July 28, 2009, 12:53 a.m.
On Mon, Jul 27, 2009 at 4:44 AM, Igor Mammedov <niallain@gmail.com> wrote:

> Looks good to me:
>
> Acked-by: Igor Mammedov <niallain@gmail.com>
>
> Jeff Layton wrote:
> > If the referral is malformed or the hostname can't be resolved, then
> > the current code generates an oops. Fix it to handle these errors
> > gracefully.
>

This is in the cifs-2.6.git tree now.   Will give it a few days before
requesting merge upstream.

Patch

diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
index 3bb11be..606912d 100644
--- a/fs/cifs/cifs_dfs_ref.c
+++ b/fs/cifs/cifs_dfs_ref.c
@@ -55,7 +55,7 @@  void cifs_dfs_release_automount_timer(void)
  * i.e. strips from UNC trailing path that is not part of share
  * name and fixup missing '\' in the begining of DFS node refferal
  * if neccessary.
- * Returns pointer to share name on success or NULL on error.
+ * Returns pointer to share name on success or ERR_PTR on error.
  * Caller is responsible for freeing returned string.
  */
 static char *cifs_get_share_name(const char *node_name)
@@ -68,7 +68,7 @@  static char *cifs_get_share_name(const char *node_name)
 	UNC = kmalloc(len+2 /*for term null and additional \ if it's missed */,
 			 GFP_KERNEL);
 	if (!UNC)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	/* get share name and server name */
 	if (node_name[1] != '\\') {
@@ -87,7 +87,7 @@  static char *cifs_get_share_name(const char *node_name)
 		cERROR(1, ("%s: no server name end in node name: %s",
 			__func__, node_name));
 		kfree(UNC);
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	/* find sharename end */
@@ -133,6 +133,12 @@  char *cifs_compose_mount_options(const char *sb_mountdata,
 		return ERR_PTR(-EINVAL);
 
 	*devname = cifs_get_share_name(ref->node_name);
+	if (IS_ERR(*devname)) {
+		rc = PTR_ERR(*devname);
+		*devname = NULL;
+		goto compose_mount_options_err;
+	}
+
 	rc = dns_resolve_server_name_to_ip(*devname, &srvIP);
 	if (rc != 0) {
 		cERROR(1, ("%s: Failed to resolve server part of %s to IP: %d",
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fc44d31..f248688 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2544,11 +2544,20 @@  remote_path_check:
 
 			if (mount_data != mount_data_global)
 				kfree(mount_data);
+
 			mount_data = cifs_compose_mount_options(
 					cifs_sb->mountdata, full_path + 1,
 					referrals, &fake_devname);
-			kfree(fake_devname);
+
 			free_dfs_info_array(referrals, num_referrals);
+			kfree(fake_devname);
+			kfree(full_path);
+
+			if (IS_ERR(mount_data)) {
+				rc = PTR_ERR(mount_data);
+				mount_data = NULL;
+				goto mount_fail_check;
+			}
 
 			if (tcon)
 				cifs_put_tcon(tcon);
@@ -2556,8 +2565,6 @@  remote_path_check:
 				cifs_put_smb_ses(pSesInfo);
 
 			cleanup_volume_info(&volume_info);
-			FreeXid(xid);
-			kfree(full_path);
 			referral_walks_count++;
 			goto try_mount_again;
 		}