[linux-cifs-client] [PATCH] [CIFS] Prevent OOPs when mounting with remote prefixpath

Jeff Layton jlayton at redhat.com
Wed Feb 4 12:13:26 GMT 2009


On Wed, 04 Feb 2009 13:35:16 +0300
Igor Mammedov <niallain at gmail.com> wrote:

> Jeff Layton wrote:
> > On Tue, 03 Feb 2009 13:22:30 +0300
> > Igor Mammedov <niallain at gmail.com> wrote:
> > 
> >> From: Igor Mammedov <niallain at gmail.com>
> >> To: Steve French <smfrench at gmail.com>
> >> Cc: "linux-cifs-client at lists.samba.org" <linux-cifs-client at lists.samba.org>
> >> Subject: [linux-cifs-client] [PATCH] [CIFS] Prevent OOPs when mounting with remote prefixpath
> >> Date: Tue, 03 Feb 2009 13:22:30 +0300
> >> Sender: linux-cifs-client-bounces+jlayton=poochiereds.net at lists.samba.org
> >> User-Agent: Thunderbird 2.0.0.19 (X11/20081209)
> >>
> >> Managed to single out the part of  DFS root support from a big patch
> >> with a little modification.
> >> It will report error and fail to mount if perfixpath is on remote server.
> >> As well it will not scare people off with a error 
> >> 'kernel BUG at fs/cifs/cifs_dfs_ref.c:274!'.
> >>
> >>
> >> ---------
> >> Fixes OOPs with message 'kernel BUG at fs/cifs/cifs_dfs_ref.c:274!'.
> >> Check if prefixpath in accessible while we are still in cifs_mount
> >> and fail with reporting a error if we can't access prefixpath
> >> (i.e. if prefixpath is located on another server)
> >>
> >>
> >> Best regards,
> >>
> >> -------------------------
> >> Igor Mammedov,
> >> niallain "at" gmail.com
> >>
> >>
> >>
> >>
> >>
> >> >From c61a715cbeee404a1bcc2788090b4d453eb43b8e Mon Sep 17 00:00:00 2001  
> >> From: Igor Mammedov <niallain at gmail.com>
> >> Date: Tue, 3 Feb 2009 12:43:56 +0300
> >> Subject: [PATCH] [CIFS] Prevent OOPs when mounting with remote prefixpath.
> >>  	Fix OOPs with message 'kernel BUG at fs/cifs/cifs_dfs_ref.c:274!'.
> >>  	Check if prefixpath in accesible while we are still in cifs_mount
> >>  	and fail with reporting a error if we can't access prefixpath
> >>          (i.e. if prefixpath is located on another server)
> >>
> >> Signed-off-by: Igor Mammedov <niallain at gmail.com>
> >> ---
> >>  fs/cifs/cifsproto.h |    1 +
> >>  fs/cifs/connect.c   |   30 ++++++++++++++++++++++++++++++
> >>  fs/cifs/inode.c     |    2 +-
> >>  3 files changed, 32 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> >> index 382ba62..22ff09c 100644
> >> --- a/fs/cifs/cifsproto.h
> >> +++ b/fs/cifs/cifsproto.h
> >> @@ -42,6 +42,7 @@ extern void _FreeXid(unsigned int);
> >>  #define GetXid() (int)_GetXid(); cFYI(1,("CIFS VFS: in %s as Xid: %d with uid: %d",__func__, xid,current_fsuid()));
> >>  #define FreeXid(curr_xid) {_FreeXid(curr_xid); cFYI(1,("CIFS VFS: leaving %s (xid = %d) rc = %d",__func__,curr_xid,(int)rc));}
> >>  extern char *build_path_from_dentry(struct dentry *);
> >> +extern char *build_path_to_root(struct cifs_sb_info *cifs_sb);
> >>  extern char *build_wildcard_path_from_dentry(struct dentry *direntry);
> >>  /* extern void renew_parental_timestamps(struct dentry *direntry);*/
> >>  extern int SendReceive(const unsigned int /* xid */ , struct cifsSesInfo *,
> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> index 2209be9..dcffaa9 100644
> >> --- a/fs/cifs/connect.c
> >> +++ b/fs/cifs/connect.c
> >> @@ -2181,6 +2181,24 @@ static void setup_cifs_sb(struct smb_vol *pvolume_info,
> >>  }
> >>  
> >>  int
> >> +is_path_accessible(int xid, struct cifsTconInfo *tcon,
> >> +		struct cifs_sb_info *cifs_sb,  const char *full_path)
> >> +{
> >> +	int rc;
> >> +	FILE_ALL_INFO *pfindData;
> >> +	pfindData = kmalloc(sizeof(FILE_ALL_INFO), GFP_KERNEL);
> >> +	if (pfindData == NULL)
> >> +		return -ENOMEM;
> >> +
> >> +	rc = CIFSSMBQPathInfo(xid, tcon, full_path, pfindData,
> >> +			0 /* not legacy */,
> >> +			cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
> >> +			CIFS_MOUNT_MAP_SPECIAL_CHR);
> >> +	kfree(pfindData);
> >> +	return rc;
> >> +}
> >> +
> >> +int
> >>  cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
> >>  	   char *mount_data, const char *devname)
> >>  {
> >> @@ -2190,6 +2208,7 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
> >>  	struct cifsSesInfo *pSesInfo = NULL;
> >>  	struct cifsTconInfo *tcon = NULL;
> >>  	struct TCP_Server_Info *srvTcp = NULL;
> >> +	char *full_path;
> >>  
> >>  	xid = GetXid();
> >>  
> >> @@ -2426,6 +2445,17 @@ mount_fail_check:
> >>  		cifs_sb->rsize = min(cifs_sb->rsize,
> >>  			       (tcon->ses->server->maxBuf - MAX_CIFS_HDR_SIZE));
> >>  
> >> +
> >> +	full_path = build_path_to_root(cifs_sb);
> >> +	if (full_path != NULL) {
> >> +		rc = is_path_accessible(xid, tcon, cifs_sb, full_path);
> >> +		kfree(full_path);
> >> +		if (rc) {
> >> +			cERROR(1, ("Remote DFS root not supported"));
> > 
> >                         ^^^ aren't you making an assumption about the error here? What if the allocation in is_path_accessable failed? A more helpful error message would be nice.
> > 
> >> +			goto mount_fail_check;
> >> +		}
> >> +	}
> >> +
> >>  	/* volume_info->password is freed above when existing session found
> >>  	(in which case it is not needed anymore) but when new sesion is created
> >>  	the password ptr is put in the new session structure (in which case the
> >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> >> index bcf7b51..00c6a3f 100644
> >> --- a/fs/cifs/inode.c
> >> +++ b/fs/cifs/inode.c
> >> @@ -621,7 +621,7 @@ static const struct inode_operations cifs_ipc_inode_ops = {
> >>  	.lookup = cifs_lookup,
> >>  };
> >>  
> >> -static char *build_path_to_root(struct cifs_sb_info *cifs_sb)
> >> +char *build_path_to_root(struct cifs_sb_info *cifs_sb)
> >>  {
> >>  	int pplen = cifs_sb->prepathlen;
> >>  	int dfsplen;
> >> -- 
> >> 1.6.0.2
> >>
> > 
> > Other than the error message this seems reasonable. I assume that it's
> > been tested?
> > 
> 
> I've tested it in mine test env.
> About 'error message', Any suggestions on what should be there?
> 

Maybe something like:

    cERROR(1, ("Path %s not accessable: %d", full_path, rc));

...of course, you'd need to not kfree full_path until after this is
printed but that's not too tough.

Won't this error also fire when trying to mount with an inaccessable
prefixpath, even if DFS is involved? If so, then mentioning DFS in
the error is probably going to be confusing for users/admins.

Actually, there's another problem too. If build_path_to_root ends up
returning NULL, then we'll just skip this check. Shouldn't we return
-ENOMEM or something at that point? It's probably better to fail the
mount than to leave the client subject to a later oops...

-- 
Jeff Layton <jlayton at redhat.com>


More information about the linux-cifs-client mailing list