пн, 25 июн. 2018 г. в 5:08, Lars Persson <lars.persson@xxxxxxxx>:
>
> With protocol version 2.0 mounts we have seen crashes with corrupt mid
> entries. Either the server->pending_mid_q list becomes corrupt with a
> cyclic reference in one element or a mid object fetched by the
> demultiplexer thread becomes overwritten during use.
>
> Code review identified a race between the demultiplexer thread and the
> request issuing thread. The demultiplexer thread seems to be written
> with the assumption that it is the sole user of the mid object until
> it calls the mid callback which either wakes the issuer task or
> deletes the mid.
>
> This assumption is not true because the issuer task can be woken up
> earlier by a signal. If the demultiplexer thread has proceeded as far
> as setting the mid_state to MID_RESPONSE_RECEIVED then the issuer
> thread will happily end up calling cifs_delete_mid while the
> demultiplexer thread still is using the mid object.
>
> Inserting a delay in the cifs demultiplexer thread widens the race
> window and makes reproduction of the race very easy:
>
> if (server->large_buf)
> buf = server->bigbuf;
>
> + usleep_range(500, 4000);
>
> server->lstrp = jiffies;
>
> To resolve this I think the proper solution involves putting a
> reference count on the mid object. This patch makes sure that the
> demultiplexer thread holds a reference until it has finished
> processing the transaction.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Lars Persson <larper@xxxxxxxx>
> ---
> Patch changelog:
> V2: Fixed possible use of an uninitialized mid_entry in cifs_demultiplex_thread.
> ---
> fs/cifs/cifsglob.h | 1 +
> fs/cifs/cifsproto.h | 1 +
> fs/cifs/connect.c | 8 +++++++-
> fs/cifs/smb1ops.c | 1 +
> fs/cifs/smb2ops.c | 1 +
> fs/cifs/smb2transport.c | 1 +
> fs/cifs/transport.c | 18 +++++++++++++++++-
> 7 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 0543187fe707..0c0b062de2ec 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1416,6 +1416,7 @@ typedef int (mid_handle_t)(struct TCP_Server_Info *server,
> /* one of these for every pending CIFS request to the server */
> struct mid_q_entry {
> struct list_head qhead; /* mids waiting on reply from this server */
> + struct kref refcount;
> struct TCP_Server_Info *server; /* server corresponding to this mid */
> __u64 mid; /* multiplex id */
> __u32 pid; /* process id */
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index e23eec5372df..7ead1a9ac6fb 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -82,6 +82,7 @@ extern struct mid_q_entry *AllocMidQEntry(const struct smb_hdr *smb_buffer,
> struct TCP_Server_Info *server);
> extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
> extern void cifs_delete_mid(struct mid_q_entry *mid);
> +extern void cifs_mid_q_entry_release(struct mid_q_entry *midEntry);
> extern void cifs_wake_up_task(struct mid_q_entry *mid);
> extern int cifs_handle_standard(struct TCP_Server_Info *server,
> struct mid_q_entry *mid);
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 73496ec316b6..842f45859968 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -924,6 +924,7 @@ cifs_demultiplex_thread(void *p)
> server->pdu_size = next_offset;
> }
>
> + mid_entry = NULL;
> if (server->ops->is_transform_hdr &&
> server->ops->receive_transform &&
> server->ops->is_transform_hdr(buf)) {
> @@ -938,8 +939,11 @@ cifs_demultiplex_thread(void *p)
> length = mid_entry->receive(server, mid_entry);
> }
>
> - if (length < 0)
> + if (length < 0) {
> + if (mid_entry)
> + cifs_mid_q_entry_release(mid_entry);
> continue;
> + }
>
> if (server->large_buf)
> buf = server->bigbuf;
> @@ -956,6 +960,8 @@ cifs_demultiplex_thread(void *p)
>
> if (!mid_entry->multiRsp || mid_entry->multiEnd)
> mid_entry->callback(mid_entry);
> +
> + cifs_mid_q_entry_release(mid_entry);
> } else if (server->ops->is_oplock_break &&
> server->ops->is_oplock_break(buf, server)) {
> cifs_dbg(FYI, "Received oplock break\n");
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index aff8ce8ba34d..646dcd149de1 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -107,6 +107,7 @@ cifs_find_mid(struct TCP_Server_Info *server, char *buffer)
> if (compare_mid(mid->mid, buf) &&
> mid->mid_state == MID_REQUEST_SUBMITTED &&
> le16_to_cpu(mid->command) == buf->Command) {
> + kref_get(&mid->refcount);
> spin_unlock(&GlobalMid_Lock);
> return mid;
> }
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 22b6449e983d..b7b8ec7df9b1 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -203,6 +203,7 @@ smb2_find_mid(struct TCP_Server_Info *server, char *buf)
> if ((mid->mid == wire_mid) &&
> (mid->mid_state == MID_REQUEST_SUBMITTED) &&
> (mid->command == shdr->Command)) {
> + kref_get(&mid->refcount);
> spin_unlock(&GlobalMid_Lock);
> return mid;
> }
> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
> index 4fe06dbc395d..719d55e63d88 100644
> --- a/fs/cifs/smb2transport.c
> +++ b/fs/cifs/smb2transport.c
> @@ -587,6 +587,7 @@ smb2_mid_entry_alloc(const struct smb2_sync_hdr *shdr,
>
> temp = mempool_alloc(cifs_mid_poolp, GFP_NOFS);
> memset(temp, 0, sizeof(struct mid_q_entry));
> + kref_init(&temp->refcount);
> temp->mid = le64_to_cpu(shdr->MessageId);
> temp->pid = current->pid;
> temp->command = shdr->Command; /* Always LE */
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index fb57dfbfb749..208ecb830466 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -61,6 +61,7 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
>
> temp = mempool_alloc(cifs_mid_poolp, GFP_NOFS);
> memset(temp, 0, sizeof(struct mid_q_entry));
> + kref_init(&temp->refcount);
> temp->mid = get_mid(smb_buffer);
> temp->pid = current->pid;
> temp->command = cpu_to_le16(smb_buffer->Command);
> @@ -82,6 +83,21 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
> return temp;
> }
>
> +static void _cifs_mid_q_entry_release(struct kref *refcount)
> +{
> + struct mid_q_entry *mid = container_of(refcount, struct mid_q_entry,
> + refcount);
> +
> + mempool_free(mid, cifs_mid_poolp);
> +}
> +
> +void cifs_mid_q_entry_release(struct mid_q_entry *midEntry)
> +{
> + spin_lock(&GlobalMid_Lock);
> + kref_put(&midEntry->refcount, _cifs_mid_q_entry_release);
> + spin_unlock(&GlobalMid_Lock);
> +}
> +
> void
> DeleteMidQEntry(struct mid_q_entry *midEntry)
> {
> @@ -110,7 +126,7 @@ DeleteMidQEntry(struct mid_q_entry *midEntry)
> }
> }
> #endif
> - mempool_free(midEntry, cifs_mid_poolp);
> + cifs_mid_q_entry_release(midEntry);
> }
>
> void
> --
> 2.17.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Didn't notice the 2nd version of the patch - looks correct.
Reviewed-by: Pavel Shilovsky <pshilov@xxxxxxxxxxxxx>
--
Best regards,
Pavel Shilovsky
