[PATCH 0/4] cifs: fix oopses and mem corruption with concurrent mount/umount (try #3) - Kernel

This is a discussion on [PATCH 0/4] cifs: fix oopses and mem corruption with concurrent mount/umount (try #3) - Kernel ; This patchset is intended to fix the oopses, memory corruption and mount failures when using the reproducer detailed here: https://bugzilla.samba.org/show_bug.cgi?id=5720 This is the third attempt at this. Since the second attempt, Steve has committed the patch to remove kthread_stop from ...

+ Reply to Thread
Results 1 to 2 of 2

Thread: [PATCH 0/4] cifs: fix oopses and mem corruption with concurrent mount/umount (try #3)

  1. [PATCH 0/4] cifs: fix oopses and mem corruption with concurrent mount/umount (try #3)

    This patchset is intended to fix the oopses, memory corruption and mount
    failures when using the reproducer detailed here:

    https://bugzilla.samba.org/show_bug.cgi?id=5720

    This is the third attempt at this. Since the second attempt, Steve has
    committed the patch to remove kthread_stop from the cifsd shutdown
    codepath. That will fix the deadlocks, but opens the window wider for
    the oopses and memory corruption in this code.

    The main difference in the remaining patches is that this set combines
    the patches that disable the structure sharing with the one that
    re-enables the TCP session sharing. It also adds a new patch that
    re-enables the sharing of SMB sessions. With this set, only tree
    connections are no longer shared.

    With this, I've been able to run the reproducer in the above BZ for
    several hours, whereas before it would regularly crash after just a few
    minutes.

    Jeff Layton (4):
    cifs: clean up server protocol handling for TCP_Server_Info
    cifs: handle the TCP_Server_Info->tsk field more carefully
    cifs: disable sharing session and tcon and add new TCP sharing code
    cifs: reinstate sharing of SMB sessions

    fs/cifs/cifs_debug.c | 54 ++++----
    fs/cifs/cifsfs.c | 25 +++--
    fs/cifs/cifsglob.h | 20 +--
    fs/cifs/cifssmb.c | 20 +---
    fs/cifs/connect.c | 354 +++++++++++++++++++++++---------------------------
    fs/cifs/misc.c | 15 +--
    6 files changed, 223 insertions(+), 265 deletions(-)

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. [PATCH 2/4] cifs: handle the TCP_Server_Info->tsk field more carefully

    We currently handle the TCP_Server_Info->tsk field without any locking,
    but with some half-measures to try and prevent races. These aren't
    really sufficient though. When taking down cifsd, use xchg() to swap
    the contents of the tsk field with NULL so we don't end up trying
    to send it more than one signal. Also, don't allow cifsd to exit until
    the signal is received if we expect one.

    Signed-off-by: Jeff Layton
    ---
    fs/cifs/connect.c | 41 ++++++++++++++++++++++++++++-------------
    1 files changed, 28 insertions(+), 13 deletions(-)

    diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
    index bf222e1..0b2adef 100644
    --- a/fs/cifs/connect.c
    +++ b/fs/cifs/connect.c
    @@ -749,6 +749,7 @@ multi_t2_fnd:
    write_unlock(&GlobalSMBSeslock);

    kfree(server->hostname);
    + task_to_wake = xchg(&server->tsk, NULL);
    kfree(server);

    length = atomic_dec_return(&tcpSesAllocCount);
    @@ -756,6 +757,16 @@ multi_t2_fnd:
    mempool_resize(cifs_req_poolp, length + cifs_min_rcv,
    GFP_KERNEL);

    + /* if server->tsk was NULL then wait for a signal before exiting */
    + if (!task_to_wake) {
    + set_current_state(TASK_INTERRUPTIBLE);
    + while (!signal_pending(current)) {
    + schedule();
    + set_current_state(TASK_INTERRUPTIBLE);
    + }
    + set_current_state(TASK_RUNNING);
    + }
    +
    return 0;
    }

    @@ -1841,6 +1852,16 @@ convert_delimiter(char *path, char delim)
    }
    }

    +static void
    +kill_cifsd(struct TCP_Server_Info *server)
    +{
    + struct task_struct *task;
    +
    + task = xchg(&server->tsk, NULL);
    + if (task)
    + force_sig(SIGKILL, task);
    +}
    +
    int
    cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
    char *mount_data, const char *devname)
    @@ -2226,7 +2247,7 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
    spin_lock(&GlobalMid_Lock);
    srvTcp->tcpStatus = CifsExiting;
    spin_unlock(&GlobalMid_Lock);
    - force_sig(SIGKILL, srvTcp->tsk);
    + kill_cifsd(srvTcp);
    }
    /* If find_unc succeeded then rc == 0 so we can not end */
    if (tcon) /* up accidently freeing someone elses tcon struct */
    @@ -2239,19 +2260,15 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
    temp_rc = CIFSSMBLogoff(xid, pSesInfo);
    /* if the socketUseCount is now zero */
    if ((temp_rc == -ESHUTDOWN) &&
    - (pSesInfo->server) &&
    - (pSesInfo->server->tsk))
    - force_sig(SIGKILL,
    - pSesInfo->server->tsk);
    + (pSesInfo->server))
    + kill_cifsd(pSesInfo->server);
    } else {
    cFYI(1, ("No session or bad tcon"));
    - if ((pSesInfo->server) &&
    - (pSesInfo->server->tsk)) {
    + if (pSesInfo->server) {
    spin_lock(&GlobalMid_Lock);
    srvTcp->tcpStatus = CifsExiting;
    spin_unlock(&GlobalMid_Lock);
    - force_sig(SIGKILL,
    - pSesInfo->server->tsk);
    + kill_cifsd(pSesInfo->server);
    }
    }
    sesInfoFree(pSesInfo);
    @@ -3538,7 +3555,6 @@ cifs_umount(struct super_block *sb, struct cifs_sb_info *cifs_sb)
    int rc = 0;
    int xid;
    struct cifsSesInfo *ses = NULL;
    - struct task_struct *cifsd_task;
    char *tmp;

    xid = GetXid();
    @@ -3554,7 +3570,6 @@ cifs_umount(struct super_block *sb, struct cifs_sb_info *cifs_sb)
    tconInfoFree(cifs_sb->tcon);
    if ((ses) && (ses->server)) {
    /* save off task so we do not refer to ses later */
    - cifsd_task = ses->server->tsk;
    cFYI(1, ("About to do SMBLogoff "));
    rc = CIFSSMBLogoff(xid, ses);
    if (rc == -EBUSY) {
    @@ -3562,8 +3577,8 @@ cifs_umount(struct super_block *sb, struct cifs_sb_info *cifs_sb)
    return 0;
    } else if (rc == -ESHUTDOWN) {
    cFYI(1, ("Waking up socket by sending signal"));
    - if (cifsd_task)
    - force_sig(SIGKILL, cifsd_task);
    + if (ses->server)
    + kill_cifsd(ses->server);
    rc = 0;
    } /* else - we have an smb session
    left on this socket do not kill cifsd */
    --
    1.5.5.1

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread