| Message ID | 4989975A.2040103@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 04 Feb 2009 16:25:46 +0300 Igor Mammedov <niallain@gmail.com> wrote: > Patch with Jeff's suggestions is attached. > > Jeff Layton wrote: > > On Wed, 04 Feb 2009 13:35:16 +0300 > ... > > > > 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... > > > Looks good to me: Acked-by: Jeff Layton <jlayton@redhat.com>
Have to get rid of the "sparse" warning (CHECK fs/cifs/connect.c fs/cifs/connect.c:2184:1: warning: symbol 'is_path_accessible' was not declared. Should it be static?) and also verify that this does not cause an extra QueryPathInfo on every mount in the normal path. On Wed, Feb 4, 2009 at 7:31 AM, Jeff Layton <jlayton@redhat.com> wrote: > On Wed, 04 Feb 2009 16:25:46 +0300 > Igor Mammedov <niallain@gmail.com> wrote: > > > Patch with Jeff's suggestions is attached. > > > > Jeff Layton wrote: > > > On Wed, 04 Feb 2009 13:35:16 +0300 > > ... > > > > > > 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... > > > > > > > Looks good to me: > > Acked-by: Jeff Layton <jlayton@redhat.com> >
Trying it just now, the patch causes an extra QueryPathInfo on every mount, and presumably it can't be cached information since it doesn't use the normal Unix vs. Non-Unix Query Path Info check and because we special cause the lookup of the root inode. On Sat, Feb 7, 2009 at 8:34 PM, Steve French <smfrench@gmail.com> wrote: > Have to get rid of the "sparse" warning (CHECK fs/cifs/connect.c > fs/cifs/connect.c:2184:1: warning: symbol 'is_path_accessible' was not > declared. Should it be static?) > > and also verify that this does not cause an extra QueryPathInfo on every > mount in the normal path. > > > On Wed, Feb 4, 2009 at 7:31 AM, Jeff Layton <jlayton@redhat.com> wrote: > >> On Wed, 04 Feb 2009 16:25:46 +0300 >> Igor Mammedov <niallain@gmail.com> wrote: >> >> > Patch with Jeff's suggestions is attached. >> > >> > Jeff Layton wrote: >> > > On Wed, 04 Feb 2009 13:35:16 +0300 >> > ... >> > > >> > > 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... >> > > >> > >> >> Looks good to me: >> >> Acked-by: Jeff Layton <jlayton@redhat.com> >> > > > > -- > Thanks, > > Steve >
On Sat, 7 Feb 2009 20:37:42 -0600 Steve French <smfrench@gmail.com> wrote: > Trying it just now, the patch causes an extra QueryPathInfo on every mount, > and presumably it can't be cached information since it doesn't use the > normal Unix vs. Non-Unix Query Path Info check and because we special cause > the lookup of the root inode. > Good point. Now that I look at this closer, I wonder whether it might be better to move this check into cifs_iget. Mounting is a pretty slow codepath anyway so an extra QPathInfo call shouldn't be a big deal. This patch is probably fine as an interim workaround, but long term, we really need to consider how best to fix this the right way -- i.e. to chase the DFS referrals in the mount() call and have it mount the ultimate target. The *best* outcome would be to handle this situation something like this: 1) parse mount options 2) find or set up tcp ses, smb ses, and tcon. 3) QPathInfo call against root inode. If it gets an -EREMOTE, chase the referral, and plug the right info into the smb_vol that we've already parsed 4) get new references to tcp_ses/smb_ses/tcon for DFS target. Do another QPathInfo call. If we get -EREMOTE, redo the setup. Repeat and keep track of the tcons we get. 5) eventually, when we hit the target we put all of the tcon references except for the last one, and plug the last tcon into the superblock. We'd probably also want to have some sort of check for too many DFS references (maybe use MAX_NESTED_LINKS) and bail out with -ELOOP or something if we get too many. This will require a bit of reorganization/breakup of cifs_mount, particularly with how option parsing is handled, but it's probably the best way to handle it. PS: Steve, you asked about -stable for this. I'm inclined to say no for stable since this code is still considered experimental.
>From e0ab3bd4afc8badf7faac83659d99afb91bc9e75 Mon Sep 17 00:00:00 2001
From: Igor Mammedov <niallain@gmail.com>
Date: Wed, 4 Feb 2009 16:18:27 +0300
Subject: [PATCH] [CIFS] Prevent OOPs when mounting with remote prefixpath.
Fixes OOPs with message 'kernel BUG at fs/cifs/cifs_dfs_ref.c:274!'.
Checks if the prefixpath in an accesible while we are still in cifs_mount
and fails with reporting a error if we can't access the prefixpath
Signed-off-by: Igor Mammedov <niallain@gmail.com>
---
fs/cifs/cifsproto.h | 1 +
fs/cifs/connect.c | 34 ++++++++++++++++++++++++++++++++++
fs/cifs/inode.c | 2 +-
3 files changed, 36 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..1b74728 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,21 @@ 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 = -ENOMEM;
+ goto mount_fail_check;
+ }
+ rc = is_path_accessible(xid, tcon, cifs_sb, full_path);
+ if (rc) {
+ cERROR(1, ("Path %s in not accessible: %d", full_path, rc));
+ kfree(full_path);
+ goto mount_fail_check;
+ }
+ kfree(full_path);
+
+
/* 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