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    [W:0.052 / U:31.384 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site