lkml.org 
[lkml]   [2009]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [linux-cifs-client] Re: [2.6.30-rc6] cifs_close: NULL pointer dereference
On Sat, 16 May 2009 22:13:31 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> On Sat, 16 May 2009 22:55:58 +0200
> Luca Tettamanti <kronos.it@gmail.com> wrote:
>
> > On Sat, May 16, 2009 at 06:28:13PM +0200, Luca Tettamanti wrote:
> > > Hello,
> > > I just hit a NULL pointer dereference in cifs_close while accessing a file on a
> > > remote Samba shared directory.
> > [...]
> > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> > > IP: [<ffffffffa05f105c>] cifs_close+0x1c1/0x2e8 [cifs]
> >
> > Ok, I've reproduced this on a machine with a serial port :)
> >
> > 0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
> > at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
> > 46 WARN(entry->prev->next != entry,
> > (gdb) bt
> > #0 0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
> > at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
> > #1 0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
> > at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
> > #2 0xffffffff8029d95e in __fput (file=0xffff8800ad8f6d00)
> > at /home/kronos/src/linux-2.6.git/fs/file_table.c:281
> > #3 0xffffffff8029da32 in fput (file=0xffff8800ac184210)
> > at /home/kronos/src/linux-2.6.git/fs/file_table.c:227
> > #4 0xffffffff8029ad30 in filp_close (filp=0xffff8800ad8f6d00, id=0xffff880037a3e080)
> > at /home/kronos/src/linux-2.6.git/fs/open.c:1108
> > #5 0xffffffff8029ade0 in sys_close (fd=0)
> > at /home/kronos/src/linux-2.6.git/fs/open.c:1137
> > #6 0xffffffff802271e4 in sysenter_dispatch ()
> > at /home/kronos/src/linux-2.6.git/arch/x86/ia32/ia32entry.S:161
> > #7 0x0000000000000000 in ?? ()
> >
> > (gdb) f 1
> > #1 0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
> > at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
> > 670 list_del(&pSMBFile->flist);
> >
> > (gdb) p *&pSMBFile->flist
> > $2 = {next = 0x0, prev = 0x0}
> >
> > (gdb) p *&pSMBFile->tlist
> > $5 = {next = 0x0, prev = 0x0}
> >
> > So both flist and tlist were not initilized in cifs_open.
> >
> > The content of the whole pSMBFile structure:
> >
> > (gdb) p *pSMBFile
> > $6 = {tlist = {next = 0x0, prev = 0x0}, flist = {next = 0x0, prev = 0x0}, uid = 0, pid = 5322, netfid = 13880,
> > pfile = 0xffff8800ad8f6d00, pInode = 0xffff8800b8d5f088, lock_mutex = {count = {counter = 1}, wait_lock = {raw_lock = {
> > slock = 514}, magic = 3735899821, owner_cpu = 4294967295, owner = 0xffffffffffffffff, dep_map = {
> > key = 0xffffffff806a5c98, class_cache = 0x0, name = 0xffffffff80537308 "&lock->wait_lock"}}, wait_list = {
> > next = 0xffff8800ac184278, prev = 0xffff8800ac184278}, owner = 0x0, name = 0x0, magic = 0xffff8800ac184240, dep_map = {
> > key = 0xffffffff80d7ff50, class_cache = 0xffffffff80835400, name = 0xffffffff8054cdfa "&private_data->lock_mutex"}},
> > llist = {next = 0xffff8800ac1842b8, prev = 0xffff8800ac1842b8}, closePend = true, invalidHandle = false,
> > messageMode = false, wrtPending = {counter = 0}, fh_mutex = {count = {counter = 1}, wait_lock = {raw_lock = {slock = 0},
> > magic = 3735899821, owner_cpu = 4294967295, owner = 0xffffffffffffffff, dep_map = {key = 0xffffffff806a5c98,
> > class_cache = 0x0, name = 0xffffffff80537308 "&lock->wait_lock"}}, wait_list = {next = 0xffff8800ac184308,
> > prev = 0xffff8800ac184308}, owner = 0x0, name = 0x0, magic = 0xffff8800ac1842d0, dep_map = {key = 0xffffffff80d7ff58,
> > class_cache = 0x0, name = 0xffffffff8054cde2 "&private_data->fh_mutex"}}, srch_inf = {index_of_last_entry = 0,
> > entries_in_buffer = 0, info_level = 0, resume_key = 0, ntwrk_buf_start = 0x0, srch_entries_start = 0x0, last_entry = 0x0,
> > presume_name = 0x0, resume_name_len = 0, endOfSearch = false, emptyDir = false, unicode = false, smallBuf = false}}
> >
>
> Luca...thanks for the bug report and analysis. I've been able to
> reproduce this too against 3.3.2. The attached patch reverts the open
> with lookup intent functionality that was added at the beginning of the
> 2.6.30 cycle. This fixes the problem for me.
>
> Steve and Shirish...given that we've already seen a couple of
> regressions from these patches, I'd like to see them reverted until
> they have been better tested.
>

Oops, wrong patch...this is the right one.

--
Jeff Layton <jlayton@redhat.com>
From 7ae9cf299fd28d23d3850ee4a4337e40139a63d7 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@redhat.com>
Date: Sat, 16 May 2009 22:08:54 -0400
Subject: [PATCH] cifs: revert patches to add open on lookup

This patch reverts the patches for open on lookup functionality
that was added at the beginning of the 2.6.30 cycle. Specifically,
these commits:

a6ce4932fbdbcd8f8e8c6df76812014351c32892
bc8cd4390c9129fbd286b10fa99972dfb68cd069
88dd47fff4891545bfcfdf39146dde8380771766
90e4ee5d311d4e0729daa676b1d7f754265b5874

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
fs/cifs/cifsglob.h | 2 +-
fs/cifs/dir.c | 154 ++++++++++++++++++----------------------------------
fs/cifs/file.c | 77 ++++++++++++++++----------
3 files changed, 100 insertions(+), 133 deletions(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index a61ab77..f1f858a 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -350,7 +350,7 @@ struct cifsFileInfo {
bool invalidHandle:1; /* file closed via session abend */
bool messageMode:1; /* for pipes: message vs byte mode */
atomic_t wrtPending; /* handle in use - defer close */
- struct mutex fh_mutex; /* prevents reopen race after dead ses*/
+ struct semaphore fh_sem; /* prevents reopen race after dead ses*/
struct cifs_search_info srch_inf;
};

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 11431ed..e457e14 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -129,62 +129,12 @@ cifs_bp_rename_retry:
return full_path;
}

-static void
-cifs_fill_fileinfo(struct inode *newinode, __u16 fileHandle,
- struct cifsTconInfo *tcon, bool write_only)
-{
- int oplock = 0;
- struct cifsFileInfo *pCifsFile;
- struct cifsInodeInfo *pCifsInode;
-
- pCifsFile = kzalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
-
- if (pCifsFile == NULL)
- return;
-
- if (oplockEnabled)
- oplock = REQ_OPLOCK;
-
- pCifsFile->netfid = fileHandle;
- pCifsFile->pid = current->tgid;
- pCifsFile->pInode = newinode;
- pCifsFile->invalidHandle = false;
- pCifsFile->closePend = false;
- mutex_init(&pCifsFile->fh_mutex);
- mutex_init(&pCifsFile->lock_mutex);
- INIT_LIST_HEAD(&pCifsFile->llist);
- atomic_set(&pCifsFile->wrtPending, 0);
-
- /* set the following in open now
- pCifsFile->pfile = file; */
- write_lock(&GlobalSMBSeslock);
- list_add(&pCifsFile->tlist, &tcon->openFileList);
- pCifsInode = CIFS_I(newinode);
- if (pCifsInode) {
- /* if readable file instance put first in list*/
- if (write_only)
- list_add_tail(&pCifsFile->flist,
- &pCifsInode->openFileList);
- else
- list_add(&pCifsFile->flist, &pCifsInode->openFileList);
-
- if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
- pCifsInode->clientCanCacheAll = true;
- pCifsInode->clientCanCacheRead = true;
- cFYI(1, ("Exclusive Oplock inode %p", newinode));
- } else if ((oplock & 0xF) == OPLOCK_READ)
- pCifsInode->clientCanCacheRead = true;
- }
- write_unlock(&GlobalSMBSeslock);
-}
-
int cifs_posix_open(char *full_path, struct inode **pinode,
struct super_block *sb, int mode, int oflags,
int *poplock, __u16 *pnetfid, int xid)
{
int rc;
__u32 oplock;
- bool write_only = false;
FILE_UNIX_BASIC_INFO *presp_data;
__u32 posix_flags = 0;
struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
@@ -222,8 +172,6 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
if (oflags & O_DIRECT)
posix_flags |= SMB_O_DIRECT;

- if (!(oflags & FMODE_READ))
- write_only = true;

rc = CIFSPOSIXCreate(xid, cifs_sb->tcon, posix_flags, mode,
pnetfid, presp_data, &oplock, full_path,
@@ -252,8 +200,6 @@ int cifs_posix_open(char *full_path, struct inode **pinode,

posix_fill_in_inode(*pinode, presp_data, 1);

- cifs_fill_fileinfo(*pinode, *pnetfid, cifs_sb->tcon, write_only);
-
posix_open_ret:
kfree(presp_data);
return rc;
@@ -281,7 +227,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
int create_options = CREATE_NOT_DIR;
int oplock = 0;
int oflags;
- bool posix_create = false;
/*
* BB below access is probably too much for mknod to request
* but we have to do query and setpathinfo so requesting
@@ -296,6 +241,7 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
char *full_path = NULL;
FILE_ALL_INFO *buf = NULL;
struct inode *newinode = NULL;
+ struct cifsInodeInfo *pCifsInode;
int disposition = FILE_OVERWRITE_IF;
bool write_only = false;

@@ -329,13 +275,11 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
negotation. EREMOTE indicates DFS junction, which is not
handled in posix open */

- if (rc == 0) {
- posix_create = true;
- if (newinode == NULL) /* query inode info */
- goto cifs_create_get_file_info;
- else /* success, no need to query */
- goto cifs_create_set_dentry;
- } else if ((rc != -EIO) && (rc != -EREMOTE) &&
+ if ((rc == 0) && (newinode == NULL))
+ goto cifs_create_get_file_info; /* query inode info */
+ else if (rc == 0) /* success, no need to query */
+ goto cifs_create_set_dentry;
+ else if ((rc != -EIO) && (rc != -EREMOTE) &&
(rc != -EOPNOTSUPP)) /* path not found or net err */
goto cifs_create_out;
/* else fallthrough to retry, using older open call, this is
@@ -467,9 +411,45 @@ cifs_create_set_dentry:
if ((nd == NULL) || (!(nd->flags & LOOKUP_OPEN))) {
/* mknod case - do not leave file open */
CIFSSMBClose(xid, tcon, fileHandle);
- } else if (!(posix_create) && (newinode)) {
- cifs_fill_fileinfo(newinode, fileHandle,
- cifs_sb->tcon, write_only);
+ } else if (newinode) {
+ struct cifsFileInfo *pCifsFile =
+ kzalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
+
+ if (pCifsFile == NULL)
+ goto cifs_create_out;
+ pCifsFile->netfid = fileHandle;
+ pCifsFile->pid = current->tgid;
+ pCifsFile->pInode = newinode;
+ pCifsFile->invalidHandle = false;
+ pCifsFile->closePend = false;
+ init_MUTEX(&pCifsFile->fh_sem);
+ mutex_init(&pCifsFile->lock_mutex);
+ INIT_LIST_HEAD(&pCifsFile->llist);
+ atomic_set(&pCifsFile->wrtPending, 0);
+
+ /* set the following in open now
+ pCifsFile->pfile = file; */
+ write_lock(&GlobalSMBSeslock);
+ list_add(&pCifsFile->tlist, &tcon->openFileList);
+ pCifsInode = CIFS_I(newinode);
+ if (pCifsInode) {
+ /* if readable file instance put first in list*/
+ if (write_only) {
+ list_add_tail(&pCifsFile->flist,
+ &pCifsInode->openFileList);
+ } else {
+ list_add(&pCifsFile->flist,
+ &pCifsInode->openFileList);
+ }
+ if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
+ pCifsInode->clientCanCacheAll = true;
+ pCifsInode->clientCanCacheRead = true;
+ cFYI(1, ("Exclusive Oplock inode %p",
+ newinode));
+ } else if ((oplock & 0xF) == OPLOCK_READ)
+ pCifsInode->clientCanCacheRead = true;
+ }
+ write_unlock(&GlobalSMBSeslock);
}
cifs_create_out:
kfree(buf);
@@ -602,21 +582,17 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, int mode,
return rc;
}

+
struct dentry *
cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
struct nameidata *nd)
{
int xid;
int rc = 0; /* to get around spurious gcc warning, set to zero here */
- int oplock = 0;
- int mode;
- __u16 fileHandle = 0;
- bool posix_open = false;
struct cifs_sb_info *cifs_sb;
struct cifsTconInfo *pTcon;
struct inode *newInode = NULL;
char *full_path = NULL;
- struct file *filp;

xid = GetXid();

@@ -658,37 +634,12 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
}
cFYI(1, ("Full path: %s inode = 0x%p", full_path, direntry->d_inode));

- if (pTcon->unix_ext) {
- if (!(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY)) &&
- (nd->flags & LOOKUP_OPEN)) {
- if (!((nd->intent.open.flags & O_CREAT) &&
- (nd->intent.open.flags & O_EXCL))) {
- mode = nd->intent.open.create_mode &
- ~current_umask();
- rc = cifs_posix_open(full_path, &newInode,
- parent_dir_inode->i_sb, mode,
- nd->intent.open.flags, &oplock,
- &fileHandle, xid);
- /*
- * This code works around a bug in
- * samba posix open in samba versions 3.3.1
- * and earlier where create works
- * but open fails with invalid parameter.
- * If either of these error codes are
- * returned, follow the normal lookup.
- * Otherwise, the error during posix open
- * is handled.
- */
- if ((rc != -EINVAL) && (rc != -EOPNOTSUPP))
- posix_open = true;
- }
- }
- if (!posix_open)
- rc = cifs_get_inode_info_unix(&newInode, full_path,
- parent_dir_inode->i_sb, xid);
- } else
+ if (pTcon->unix_ext)
+ rc = cifs_get_inode_info_unix(&newInode, full_path,
+ parent_dir_inode->i_sb, xid);
+ else
rc = cifs_get_inode_info(&newInode, full_path, NULL,
- parent_dir_inode->i_sb, xid, NULL);
+ parent_dir_inode->i_sb, xid, NULL);

if ((rc == 0) && (newInode != NULL)) {
if (pTcon->nocase)
@@ -696,8 +647,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
else
direntry->d_op = &cifs_dentry_ops;
d_add(direntry, newInode);
- if (posix_open)
- filp = lookup_instantiate_filp(nd, direntry, NULL);
+
/* since paths are not looked up by component - the parent
directories are presumed to be good here */
renew_parental_timestamps(direntry);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 38c06f8..dfd3e6c 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -46,7 +46,7 @@ static inline struct cifsFileInfo *cifs_init_private(
memset(private_data, 0, sizeof(struct cifsFileInfo));
private_data->netfid = netfid;
private_data->pid = current->tgid;
- mutex_init(&private_data->fh_mutex);
+ init_MUTEX(&private_data->fh_sem);
mutex_init(&private_data->lock_mutex);
INIT_LIST_HEAD(&private_data->llist);
private_data->pfile = file; /* needed for writepage */
@@ -129,12 +129,15 @@ static inline int cifs_posix_open_inode_helper(struct inode *inode,
struct file *file, struct cifsInodeInfo *pCifsInode,
struct cifsFileInfo *pCifsFile, int oplock, u16 netfid)
{
+ struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+/* struct timespec temp; */ /* BB REMOVEME BB */

file->private_data = kmalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
if (file->private_data == NULL)
return -ENOMEM;
pCifsFile = cifs_init_private(file->private_data, inode, file, netfid);
write_lock(&GlobalSMBSeslock);
+ list_add(&pCifsFile->tlist, &cifs_sb->tcon->openFileList);

pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
if (pCifsInode == NULL) {
@@ -142,6 +145,17 @@ static inline int cifs_posix_open_inode_helper(struct inode *inode,
return -EINVAL;
}

+ /* want handles we can use to read with first
+ in the list so we do not have to walk the
+ list to search for one in write_begin */
+ if ((file->f_flags & O_ACCMODE) == O_WRONLY) {
+ list_add_tail(&pCifsFile->flist,
+ &pCifsInode->openFileList);
+ } else {
+ list_add(&pCifsFile->flist,
+ &pCifsInode->openFileList);
+ }
+
if (pCifsInode->clientCanCacheRead) {
/* we have the inode open somewhere else
no need to discard cache data */
@@ -270,32 +284,35 @@ int cifs_open(struct inode *inode, struct file *file)
cifs_sb = CIFS_SB(inode->i_sb);
tcon = cifs_sb->tcon;

- /* search inode for this file and fill in file->private_data */
- pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
- read_lock(&GlobalSMBSeslock);
- list_for_each(tmp, &pCifsInode->openFileList) {
- pCifsFile = list_entry(tmp, struct cifsFileInfo,
- flist);
- if ((pCifsFile->pfile == NULL) &&
- (pCifsFile->pid == current->tgid)) {
- /* mode set in cifs_create */
-
- /* needed for writepage */
- pCifsFile->pfile = file;
-
- file->private_data = pCifsFile;
- break;
+ if (file->f_flags & O_CREAT) {
+ /* search inode for this file and fill in file->private_data */
+ pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
+ read_lock(&GlobalSMBSeslock);
+ list_for_each(tmp, &pCifsInode->openFileList) {
+ pCifsFile = list_entry(tmp, struct cifsFileInfo,
+ flist);
+ if ((pCifsFile->pfile == NULL) &&
+ (pCifsFile->pid == current->tgid)) {
+ /* mode set in cifs_create */
+
+ /* needed for writepage */
+ pCifsFile->pfile = file;
+
+ file->private_data = pCifsFile;
+ break;
+ }
+ }
+ read_unlock(&GlobalSMBSeslock);
+ if (file->private_data != NULL) {
+ rc = 0;
+ FreeXid(xid);
+ return rc;
+ } else {
+ if (file->f_flags & O_EXCL)
+ cERROR(1, ("could not find file instance for "
+ "new file %p", file));
}
}
- read_unlock(&GlobalSMBSeslock);
-
- if (file->private_data != NULL) {
- rc = 0;
- FreeXid(xid);
- return rc;
- } else if ((file->f_flags & O_CREAT) && (file->f_flags & O_EXCL))
- cERROR(1, ("could not find file instance for "
- "new file %p", file));

full_path = build_path_from_dentry(file->f_path.dentry);
if (full_path == NULL) {
@@ -483,9 +500,9 @@ static int cifs_reopen_file(struct file *file, bool can_flush)
return -EBADF;

xid = GetXid();
- mutex_unlock(&pCifsFile->fh_mutex);
+ down(&pCifsFile->fh_sem);
if (!pCifsFile->invalidHandle) {
- mutex_lock(&pCifsFile->fh_mutex);
+ up(&pCifsFile->fh_sem);
FreeXid(xid);
return 0;
}
@@ -516,7 +533,7 @@ static int cifs_reopen_file(struct file *file, bool can_flush)
if (full_path == NULL) {
rc = -ENOMEM;
reopen_error_exit:
- mutex_lock(&pCifsFile->fh_mutex);
+ up(&pCifsFile->fh_sem);
FreeXid(xid);
return rc;
}
@@ -558,14 +575,14 @@ reopen_error_exit:
cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
CIFS_MOUNT_MAP_SPECIAL_CHR);
if (rc) {
- mutex_lock(&pCifsFile->fh_mutex);
+ up(&pCifsFile->fh_sem);
cFYI(1, ("cifs_open returned 0x%x", rc));
cFYI(1, ("oplock: %d", oplock));
} else {
reopen_success:
pCifsFile->netfid = netfid;
pCifsFile->invalidHandle = false;
- mutex_lock(&pCifsFile->fh_mutex);
+ up(&pCifsFile->fh_sem);
pCifsInode = CIFS_I(inode);
if (pCifsInode) {
if (can_flush) {
--
1.6.2.2
\
 
 \ /
  Last update: 2009-05-17 04:19    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site