| Message ID | 20180614083809.3596-1-larper@axis.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: Fix use after free of a mid_q_entry | expand |
Looks plausible - but would like additional feedback if others have opinions On Thu, Jun 14, 2018 at 3:38 AM, Lars Persson <lars.persson@axis.com> wrote: > 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. > > Signed-off-by: Lars Persson <larper@axis.com> > --- > fs/cifs/cifsglob.h | 1 + > fs/cifs/cifsproto.h | 1 + > fs/cifs/connect.c | 7 ++++++- > fs/cifs/smb1ops.c | 1 + > fs/cifs/smb2ops.c | 1 + > fs/cifs/smb2transport.c | 1 + > fs/cifs/transport.c | 18 +++++++++++++++++- > 7 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index cb950a5fa078..c7ee09d9a236 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -1362,6 +1362,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 365a414a75e9..c4e5c69810f9 100644 > --- a/fs/cifs/cifsproto.h > +++ b/fs/cifs/cifsproto.h > @@ -76,6 +76,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 7a10a5d0731f..90cedf6b3228 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -920,8 +920,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; > @@ -938,6 +941,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 9c6d95ffca97..ba0bc31786d1 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 8806f3f76c1d..97f24d82ae6b 100644 > --- a/fs/cifs/smb2transport.c > +++ b/fs/cifs/smb2transport.c > @@ -548,6 +548,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 927226a2122f..60faf2fcec7f 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi For your reference, this is a KASAN log for one of the use-after-free possibilies. ================================================================== BUG: KASAN: use-after-free in cifs_demultiplex_thread+0xc77/0x1200 Write of size 4 at addr ffff880032a29958 by task cifsd/1199 CPU: 0 PID: 1199 Comm: cifsd Tainted: G C 4.17.0+ #4 Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 Call Trace: dump_stack+0x7b/0xb5 print_address_description+0x6f/0x280 kasan_report+0x25b/0x380 ? cifs_demultiplex_thread+0xc77/0x1200 __asan_store4+0x7b/0x80 cifs_demultiplex_thread+0xc77/0x1200 ? cifs_handle_standard+0x280/0x280 ? compat_start_thread+0x60/0x60 ? kasan_check_write+0x14/0x20 ? finish_task_switch+0xf6/0x3b0 ? __schedule+0x502/0xd80 ? __sched_text_start+0x8/0x8 ? __kthread_parkme+0xcb/0x100 ? kthread_blkcg+0x70/0x70 kthread+0x180/0x1d0 ? cifs_handle_standard+0x280/0x280 ? kthread_create_worker_on_cpu+0xc0/0xc0 ret_from_fork+0x35/0x40 Allocated by task 2688: save_stack+0x46/0xd0 kasan_kmalloc+0xad/0xe0 kasan_slab_alloc+0x11/0x20 kmem_cache_alloc+0xe8/0x200 mempool_alloc_slab+0x15/0x20 mempool_alloc+0x111/0x280 smb2_mid_entry_alloc+0x3e/0x180 smb2_setup_request+0x14f/0x2b0 cifs_send_recv+0x191/0x800 smb2_send_recv+0x1b3/0x300 SMB2_open+0x8ca/0x13a0 smb2_open_op_close+0x122/0x300 smb2_query_path_info+0x85/0x110 cifs_get_inode_info+0x911/0xf00 cifs_lookup+0x434/0x670 cifs_atomic_open+0xe2/0x640 path_openat+0x1484/0x1de0 do_filp_open+0x126/0x1c0 do_sys_open+0x17d/0x2b0 __x64_sys_openat+0x59/0x70 do_syscall_64+0x78/0x170 entry_SYSCALL_64_after_hwframe+0x44/0xa9 Freed by task 2688: save_stack+0x46/0xd0 __kasan_slab_free+0x129/0x190 kasan_slab_free+0xe/0x10 kmem_cache_free+0x7c/0x210 mempool_free_slab+0x17/0x20 mempool_free+0x65/0x170 DeleteMidQEntry+0x71/0x90 cifs_delete_mid+0x7f/0x90 cifs_send_recv+0x47f/0x800 smb2_send_recv+0x1b3/0x300 SMB2_open+0x8ca/0x13a0 smb2_open_op_close+0x122/0x300 smb2_query_path_info+0x85/0x110 cifs_get_inode_info+0x911/0xf00 cifs_lookup+0x434/0x670 cifs_atomic_open+0xe2/0x640 path_openat+0x1484/0x1de0 do_filp_open+0x126/0x1c0 do_sys_open+0x17d/0x2b0 __x64_sys_openat+0x59/0x70 do_syscall_64+0x78/0x170 entry_SYSCALL_64_after_hwframe+0x44/0xa9 The buggy address belongs to the object at ffff880032a29900 which belongs to the cache cifs_mpx_ids of size 104 The buggy address is located 88 bytes inside of 104-byte region [ffff880032a29900, ffff880032a29968) The buggy address belongs to the page: page:ffffea0000ca8a40 count:1 mapcount:0 mapping:0000000000000000 index:0x0 flags: 0xfffffc0000100(slab) raw: 000fffffc0000100 0000000000000000 0000000000000000 0000000180150015 raw: ffffea000059a600 0000000a0000000a ffff880011eb1340 0000000000000000 page dumped because: kasan: bad access detected Memory state around the buggy address: ffff880032a29800: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb ffff880032a29880: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc >ffff880032a29900: fb fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc ^ ffff880032a29980: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb ffff880032a29a00: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc ================================================================== On 06/14/2018 10:38 AM, Lars Persson wrote: > 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. > > Signed-off-by: Lars Persson <larper@axis.com> > --- > fs/cifs/cifsglob.h | 1 + > fs/cifs/cifsproto.h | 1 + > fs/cifs/connect.c | 7 ++++++- > fs/cifs/smb1ops.c | 1 + > fs/cifs/smb2ops.c | 1 + > fs/cifs/smb2transport.c | 1 + > fs/cifs/transport.c | 18 +++++++++++++++++- > 7 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index cb950a5fa078..c7ee09d9a236 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -1362,6 +1362,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 365a414a75e9..c4e5c69810f9 100644 > --- a/fs/cifs/cifsproto.h > +++ b/fs/cifs/cifsproto.h > @@ -76,6 +76,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 7a10a5d0731f..90cedf6b3228 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -920,8 +920,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; > @@ -938,6 +941,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 9c6d95ffca97..ba0bc31786d1 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 8806f3f76c1d..97f24d82ae6b 100644 > --- a/fs/cifs/smb2transport.c > +++ b/fs/cifs/smb2transport.c > @@ -548,6 +548,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 927226a2122f..60faf2fcec7f 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
вт, 19 июн. 2018 г. в 0:25, Lars Persson <lars.persson@axis.com>: > > Hi > > For your reference, this is a KASAN log for one of the use-after-free possibilies. > > ================================================================== > BUG: KASAN: use-after-free in cifs_demultiplex_thread+0xc77/0x1200 > Write of size 4 at addr ffff880032a29958 by task cifsd/1199 > > CPU: 0 PID: 1199 Comm: cifsd Tainted: G C 4.17.0+ #4 > Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 > Call Trace: > dump_stack+0x7b/0xb5 > print_address_description+0x6f/0x280 > kasan_report+0x25b/0x380 > ? cifs_demultiplex_thread+0xc77/0x1200 > __asan_store4+0x7b/0x80 > cifs_demultiplex_thread+0xc77/0x1200 > ? cifs_handle_standard+0x280/0x280 > ? compat_start_thread+0x60/0x60 > ? kasan_check_write+0x14/0x20 > ? finish_task_switch+0xf6/0x3b0 > ? __schedule+0x502/0xd80 > ? __sched_text_start+0x8/0x8 > ? __kthread_parkme+0xcb/0x100 > ? kthread_blkcg+0x70/0x70 > kthread+0x180/0x1d0 > ? cifs_handle_standard+0x280/0x280 > ? kthread_create_worker_on_cpu+0xc0/0xc0 > ret_from_fork+0x35/0x40 > > Allocated by task 2688: > save_stack+0x46/0xd0 > kasan_kmalloc+0xad/0xe0 > kasan_slab_alloc+0x11/0x20 > kmem_cache_alloc+0xe8/0x200 > mempool_alloc_slab+0x15/0x20 > mempool_alloc+0x111/0x280 > smb2_mid_entry_alloc+0x3e/0x180 > smb2_setup_request+0x14f/0x2b0 > cifs_send_recv+0x191/0x800 > smb2_send_recv+0x1b3/0x300 > SMB2_open+0x8ca/0x13a0 > smb2_open_op_close+0x122/0x300 > smb2_query_path_info+0x85/0x110 > cifs_get_inode_info+0x911/0xf00 > cifs_lookup+0x434/0x670 > cifs_atomic_open+0xe2/0x640 > path_openat+0x1484/0x1de0 > do_filp_open+0x126/0x1c0 > do_sys_open+0x17d/0x2b0 > __x64_sys_openat+0x59/0x70 > do_syscall_64+0x78/0x170 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > Freed by task 2688: > save_stack+0x46/0xd0 > __kasan_slab_free+0x129/0x190 > kasan_slab_free+0xe/0x10 > kmem_cache_free+0x7c/0x210 > mempool_free_slab+0x17/0x20 > mempool_free+0x65/0x170 > DeleteMidQEntry+0x71/0x90 > cifs_delete_mid+0x7f/0x90 > cifs_send_recv+0x47f/0x800 > smb2_send_recv+0x1b3/0x300 > SMB2_open+0x8ca/0x13a0 > smb2_open_op_close+0x122/0x300 > smb2_query_path_info+0x85/0x110 > cifs_get_inode_info+0x911/0xf00 > cifs_lookup+0x434/0x670 > cifs_atomic_open+0xe2/0x640 > path_openat+0x1484/0x1de0 > do_filp_open+0x126/0x1c0 > do_sys_open+0x17d/0x2b0 > __x64_sys_openat+0x59/0x70 > do_syscall_64+0x78/0x170 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > The buggy address belongs to the object at ffff880032a29900 > which belongs to the cache cifs_mpx_ids of size 104 > The buggy address is located 88 bytes inside of > 104-byte region [ffff880032a29900, ffff880032a29968) > The buggy address belongs to the page: > page:ffffea0000ca8a40 count:1 mapcount:0 mapping:0000000000000000 index:0x0 > flags: 0xfffffc0000100(slab) > raw: 000fffffc0000100 0000000000000000 0000000000000000 0000000180150015 > raw: ffffea000059a600 0000000a0000000a ffff880011eb1340 0000000000000000 > page dumped because: kasan: bad access detected > > Memory state around the buggy address: > ffff880032a29800: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb > ffff880032a29880: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc > >ffff880032a29900: fb fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc > ^ > ffff880032a29980: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb > ffff880032a29a00: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc > ================================================================== > > > > On 06/14/2018 10:38 AM, Lars Persson wrote: > > 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. > > > > Signed-off-by: Lars Persson <larper@axis.com> > > --- > > fs/cifs/cifsglob.h | 1 + > > fs/cifs/cifsproto.h | 1 + > > fs/cifs/connect.c | 7 ++++++- > > fs/cifs/smb1ops.c | 1 + > > fs/cifs/smb2ops.c | 1 + > > fs/cifs/smb2transport.c | 1 + > > fs/cifs/transport.c | 18 +++++++++++++++++- > > 7 files changed, 28 insertions(+), 2 deletions(-) > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > > index cb950a5fa078..c7ee09d9a236 100644 > > --- a/fs/cifs/cifsglob.h > > +++ b/fs/cifs/cifsglob.h > > @@ -1362,6 +1362,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 365a414a75e9..c4e5c69810f9 100644 > > --- a/fs/cifs/cifsproto.h > > +++ b/fs/cifs/cifsproto.h > > @@ -76,6 +76,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 7a10a5d0731f..90cedf6b3228 100644 > > --- a/fs/cifs/connect.c > > +++ b/fs/cifs/connect.c > > @@ -920,8 +920,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; > > @@ -938,6 +941,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 9c6d95ffca97..ba0bc31786d1 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 8806f3f76c1d..97f24d82ae6b 100644 > > --- a/fs/cifs/smb2transport.c > > +++ b/fs/cifs/smb2transport.c > > @@ -548,6 +548,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 927226a2122f..60faf2fcec7f 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 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Thanks for catching this! Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> -- Best regards, Pavel Shilovsky -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index cb950a5fa078..c7ee09d9a236 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -1362,6 +1362,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 365a414a75e9..c4e5c69810f9 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -76,6 +76,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 7a10a5d0731f..90cedf6b3228 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -920,8 +920,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; @@ -938,6 +941,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 9c6d95ffca97..ba0bc31786d1 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 8806f3f76c1d..97f24d82ae6b 100644 --- a/fs/cifs/smb2transport.c +++ b/fs/cifs/smb2transport.c @@ -548,6 +548,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 927226a2122f..60faf2fcec7f 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
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. Signed-off-by: Lars Persson <larper@axis.com> --- fs/cifs/cifsglob.h | 1 + fs/cifs/cifsproto.h | 1 + fs/cifs/connect.c | 7 ++++++- fs/cifs/smb1ops.c | 1 + fs/cifs/smb2ops.c | 1 + fs/cifs/smb2transport.c | 1 + fs/cifs/transport.c | 18 +++++++++++++++++- 7 files changed, 28 insertions(+), 2 deletions(-)