* [bug report] Handle mismatched open calls
@ 2017-04-06 9:11 Dan Carpenter
2017-04-06 10:07 ` Sachin Prabhu
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2017-04-06 9:11 UTC (permalink / raw)
To: sprabhu-H+wXaHxf7aLQT0dZR+AlfA; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
Hello Sachin Prabhu,
The patch 96b75d0508f8: "Handle mismatched open calls" from Mar 3,
2017, leads to the following static checker warning:
fs/cifs/cifssmb.c:1530 cifs_readv_receive()
error: potential NULL dereference 'server->smallbuf'.
fs/cifs/cifssmb.c
1519 cifs_dbg(FYI, "0: iov_base=%p iov_len=%u\n",
1520 rdata->iov[0].iov_base, server->total_read);
1521
1522 mid->resp_buf = server->smallbuf;
1523 server->smallbuf = NULL;
^^^^^^^^^^^^^^^^^^^^^^^
We set this to NULL here
1524
1525 /* how much data is in the response? */
1526 data_len = server->ops->read_data_length(buf);
1527 if (data_offset + data_len > buflen) {
1528 /* data_len is corrupt -- discard frame */
1529 rdata->result = -EIO;
1530 return cifs_readv_discard(server, mid);
^^^^^^
but we need it here.
1531 }
1532
1533 length = rdata->read_into_pages(server, rdata, data_len);
1534 if (length < 0)
1535 return length;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [bug report] Handle mismatched open calls 2017-04-06 9:11 [bug report] Handle mismatched open calls Dan Carpenter @ 2017-04-06 10:07 ` Sachin Prabhu [not found] ` <1491473227.3042.2.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Sachin Prabhu @ 2017-04-06 10:07 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA On Thu, 2017-04-06 at 12:11 +0300, Dan Carpenter wrote: > Hello Sachin Prabhu, > > The patch 96b75d0508f8: "Handle mismatched open calls" from Mar 3, > 2017, leads to the following static checker warning: > > fs/cifs/cifssmb.c:1530 cifs_readv_receive() > error: potential NULL dereference 'server->smallbuf'. > > fs/cifs/cifssmb.c > 1519 cifs_dbg(FYI, "0: iov_base=%p iov_len=%u\n", > 1520 rdata->iov[0].iov_base, server->total_read); > 1521 > 1522 mid->resp_buf = server->smallbuf; > 1523 server->smallbuf = NULL; > ^^^^^^^^^^^^^^^^^^^^^^^ > We set this to NULL here > > 1524 > 1525 /* how much data is in the response? */ > 1526 data_len = server->ops->read_data_length(buf); > 1527 if (data_offset + data_len > buflen) { > 1528 /* data_len is corrupt -- discard frame */ > 1529 rdata->result = -EIO; > 1530 return cifs_readv_discard(server, mid); > ^^^^^^ > but we need it here. > > 1531 } > 1532 > 1533 length = rdata->read_into_pages(server, rdata, > data_len); > 1534 if (length < 0) > 1535 return length; > > regards, > dan carpenter Thanks Dan, We should probably move that piece of code to the bottom of the function just before we return. Sachin Prabhu ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <1491473227.3042.2.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]
* Re: [bug report] Handle mismatched open calls [not found] ` <1491473227.3042.2.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> @ 2017-04-06 18:24 ` Sachin Prabhu [not found] ` <1491503047.8010.2.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Sachin Prabhu @ 2017-04-06 18:24 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA, smfrench [-- Attachment #1: Type: text/plain, Size: 2071 bytes --] On Thu, 2017-04-06 at 11:07 +0100, Sachin Prabhu wrote: > On Thu, 2017-04-06 at 12:11 +0300, Dan Carpenter wrote: > > Hello Sachin Prabhu, > > > > The patch 96b75d0508f8: "Handle mismatched open calls" from Mar 3, > > 2017, leads to the following static checker warning: > > > > fs/cifs/cifssmb.c:1530 cifs_readv_receive() > > error: potential NULL dereference 'server->smallbuf'. > > > > fs/cifs/cifssmb.c > > 1519 cifs_dbg(FYI, "0: iov_base=%p iov_len=%u\n", > > 1520 rdata->iov[0].iov_base, server- > > >total_read); > > 1521 > > 1522 mid->resp_buf = server->smallbuf; > > 1523 server->smallbuf = NULL; > > ^^^^^^^^^^^^^^^^^^^^^^^ > > We set this to NULL here > > > > 1524 > > 1525 /* how much data is in the response? */ > > 1526 data_len = server->ops->read_data_length(buf); > > 1527 if (data_offset + data_len > buflen) { > > 1528 /* data_len is corrupt -- discard frame */ > > 1529 rdata->result = -EIO; > > 1530 return cifs_readv_discard(server, mid); > > ^^^^^^ > > but we need it here. > > > > 1531 } > > 1532 > > 1533 length = rdata->read_into_pages(server, rdata, > > data_len); > > 1534 if (length < 0) > > 1535 return length; > > > > regards, > > dan carpenter > > Thanks Dan, > Hello Dan, I have attached a patch which replaces the original "Handle mismatched open calls" patch from the for-next branch. Can you please run it against the static checker. In the patch, we pass a buffer to cifs_discard_remaining_data() instead of using server->small_buf to obtain the rfc len. Thanks Sachin Prabhu [-- Attachment #2: 0001-Handle-mismatched-open-calls.patch --] [-- Type: text/x-patch, Size: 14180 bytes --] From 83f1e5cca033c358b98ebc53389787dfa9051934 Mon Sep 17 00:00:00 2001 From: Sachin Prabhu <sprabhu@redhat.com> Date: Fri, 3 Mar 2017 15:41:38 -0800 Subject: [PATCH] Handle mismatched open calls A signal can interrupt a SendReceive call which result in incoming responses to the call being ignored. This is a problem for calls such as open which results in the successful response being ignored. This results in an open file resource on the server. The patch looks into responses which were cancelled after being sent and in case of successful open closes the open fids. For this patch, the check is only done in SendReceive2() RH-bz: 1403319 v2: Search for sess and tcon while under spinlock v3: Allocate struct cancelled before we pick the tcon v4: Rebase against latest upstream. v5: Fix oops caused by async reads v6: Pass buffer to cifs_discard_remaining_data() Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> Cc: Stable <stable@vger.kernel.org> --- fs/cifs/cifsglob.h | 11 ++++++++++ fs/cifs/cifsproto.h | 3 ++- fs/cifs/cifssmb.c | 11 ++++++---- fs/cifs/connect.c | 13 +++++++++-- fs/cifs/smb2misc.c | 48 ++++++++++++++++++++++++++++++++++++++++ fs/cifs/smb2ops.c | 8 +++++-- fs/cifs/smb2proto.h | 7 ++++++ fs/cifs/smb2transport.c | 58 +++++++++++++++++++++++++++++++++++++++++++++---- fs/cifs/transport.c | 3 +++ 9 files changed, 149 insertions(+), 13 deletions(-) diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index c565e0db..d07f13a 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -243,6 +243,7 @@ struct smb_version_operations { /* verify the message */ int (*check_message)(char *, unsigned int, struct TCP_Server_Info *); bool (*is_oplock_break)(char *, struct TCP_Server_Info *); + int (*handle_cancelled_mid)(char *, struct TCP_Server_Info *); void (*downgrade_oplock)(struct TCP_Server_Info *, struct cifsInodeInfo *, bool); /* process transaction2 response */ @@ -1344,6 +1345,7 @@ struct mid_q_entry { void *callback_data; /* general purpose pointer for callback */ void *resp_buf; /* pointer to received SMB header */ int mid_state; /* wish this were enum but can not pass to wait_event */ + unsigned int mid_flags; __le16 command; /* smb command code */ bool large_buf:1; /* if valid response, is pointer to large buf */ bool multiRsp:1; /* multiple trans2 responses for one request */ @@ -1351,6 +1353,12 @@ struct mid_q_entry { bool decrypted:1; /* decrypted entry */ }; +struct close_cancelled_open { + struct cifs_fid fid; + struct cifs_tcon *tcon; + struct work_struct work; +}; + /* Make code in transport.c a little cleaner by moving update of optional stats into function below */ #ifdef CONFIG_CIFS_STATS2 @@ -1482,6 +1490,9 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param, #define MID_RESPONSE_MALFORMED 0x10 #define MID_SHUTDOWN 0x20 +/* Flags */ +#define MID_WAIT_CANCELLED 1 /* Cancelled while waiting for response */ + /* Types of response buffer returned from SendReceive2 */ #define CIFS_NO_BUFFER 0 /* Response buffer not returned */ #define CIFS_SMALL_BUFFER 1 diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index 97e5d23..ec5e5e5 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -79,7 +79,8 @@ extern void cifs_delete_mid(struct mid_q_entry *mid); 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); -extern int cifs_discard_remaining_data(struct TCP_Server_Info *server); +extern int cifs_discard_remaining_data(struct TCP_Server_Info *server, + char *buf); extern int cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst, mid_receive_t *receive, mid_callback_t *callback, diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index 0669506..967b9263 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -1400,9 +1400,9 @@ CIFS_open(const unsigned int xid, struct cifs_open_parms *oparms, int *oplock, * current bigbuf. */ int -cifs_discard_remaining_data(struct TCP_Server_Info *server) +cifs_discard_remaining_data(struct TCP_Server_Info *server, char *buf) { - unsigned int rfclen = get_rfc1002_length(server->smallbuf); + unsigned int rfclen = get_rfc1002_length(buf); int remaining = rfclen + 4 - server->total_read; while (remaining > 0) { @@ -1426,7 +1426,7 @@ cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid) int length; struct cifs_readdata *rdata = mid->callback_data; - length = cifs_discard_remaining_data(server); + length = cifs_discard_remaining_data(server, mid->resp_buf); dequeue_mid(mid, rdata->result); return length; } @@ -1459,7 +1459,7 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid) if (server->ops->is_status_pending && server->ops->is_status_pending(buf, server, 0)) { - cifs_discard_remaining_data(server); + cifs_discard_remaining_data(server, buf); return -1; } @@ -1519,6 +1519,9 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid) cifs_dbg(FYI, "0: iov_base=%p iov_len=%u\n", rdata->iov[0].iov_base, server->total_read); + mid->resp_buf = server->smallbuf; + server->smallbuf = NULL; + /* how much data is in the response? */ data_len = server->ops->read_data_length(buf); if (data_offset + data_len > buflen) { diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 9ae695a..131663b 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -904,10 +904,19 @@ cifs_demultiplex_thread(void *p) server->lstrp = jiffies; if (mid_entry != NULL) { + if ((mid_entry->mid_flags & MID_WAIT_CANCELLED) && + mid_entry->mid_state == MID_RESPONSE_RECEIVED && + (server->ops->handle_cancelled_mid)) + server->ops->handle_cancelled_mid( + mid_entry->resp_buf, + server); + if (!mid_entry->multiRsp || mid_entry->multiEnd) mid_entry->callback(mid_entry); - } else if (!server->ops->is_oplock_break || - !server->ops->is_oplock_break(buf, server)) { + } else if (server->ops->is_oplock_break && + server->ops->is_oplock_break(buf, server)) { + cifs_dbg(FYI, "Received oplock break\n"); + } else { cifs_dbg(VFS, "No task to wake, unknown frame received! NumMids %d\n", atomic_read(&midCount)); cifs_dump_mem("Received Data is: ", buf, diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c index fd516ea..f50e3ef 100644 --- a/fs/cifs/smb2misc.c +++ b/fs/cifs/smb2misc.c @@ -659,3 +659,51 @@ smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server) cifs_dbg(FYI, "Can not process oplock break for non-existent connection\n"); return false; } + +void +smb2_cancelled_close_fid(struct work_struct *work) +{ + struct close_cancelled_open *cancelled = container_of(work, + struct close_cancelled_open, work); + + cifs_dbg(VFS, "Close unmatched open\n"); + + SMB2_close(0, cancelled->tcon, cancelled->fid.persistent_fid, + cancelled->fid.volatile_fid); + cifs_put_tcon(cancelled->tcon); + kfree(cancelled); +} + +int +smb2_handle_cancelled_mid(char *buffer, struct TCP_Server_Info *server) +{ + struct smb2_sync_hdr *sync_hdr = get_sync_hdr(buffer); + struct smb2_create_rsp *rsp = (struct smb2_create_rsp *)buffer; + struct cifs_tcon *tcon; + struct close_cancelled_open *cancelled; + + if ((sync_hdr->Command != SMB2_CREATE) || + (sync_hdr->Status != STATUS_SUCCESS)) + return 0; + + cancelled = (struct close_cancelled_open *) + kzalloc(sizeof(struct close_cancelled_open), + GFP_KERNEL); + if (!cancelled) + return -ENOMEM; + + tcon = smb2_find_smb_tcon(server, sync_hdr->SessionId, + sync_hdr->TreeId); + if (!tcon) { + kfree(cancelled); + return -ENOENT; + } + + cancelled->fid.persistent_fid = rsp->PersistentFileId; + cancelled->fid.volatile_fid = rsp->VolatileFileId; + cancelled->tcon = tcon; + INIT_WORK(&cancelled->work, smb2_cancelled_close_fid); + queue_work(cifsiod_wq, &cancelled->work); + + return 0; +} diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 583a0e2..7b12a72 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -2195,7 +2195,7 @@ receive_encrypted_read(struct TCP_Server_Info *server, struct mid_q_entry **mid) if (rc) goto free_pages; - rc = cifs_discard_remaining_data(server); + rc = cifs_discard_remaining_data(server, buf); if (rc) goto free_pages; @@ -2221,7 +2221,7 @@ receive_encrypted_read(struct TCP_Server_Info *server, struct mid_q_entry **mid) kfree(pages); return rc; discard_data: - cifs_discard_remaining_data(server); + cifs_discard_remaining_data(server, buf); goto free_pages; } @@ -2329,6 +2329,7 @@ struct smb_version_operations smb20_operations = { .clear_stats = smb2_clear_stats, .print_stats = smb2_print_stats, .is_oplock_break = smb2_is_valid_oplock_break, + .handle_cancelled_mid = smb2_handle_cancelled_mid, .downgrade_oplock = smb2_downgrade_oplock, .need_neg = smb2_need_neg, .negotiate = smb2_negotiate, @@ -2411,6 +2412,7 @@ struct smb_version_operations smb21_operations = { .clear_stats = smb2_clear_stats, .print_stats = smb2_print_stats, .is_oplock_break = smb2_is_valid_oplock_break, + .handle_cancelled_mid = smb2_handle_cancelled_mid, .downgrade_oplock = smb2_downgrade_oplock, .need_neg = smb2_need_neg, .negotiate = smb2_negotiate, @@ -2495,6 +2497,7 @@ struct smb_version_operations smb30_operations = { .print_stats = smb2_print_stats, .dump_share_caps = smb2_dump_share_caps, .is_oplock_break = smb2_is_valid_oplock_break, + .handle_cancelled_mid = smb2_handle_cancelled_mid, .downgrade_oplock = smb2_downgrade_oplock, .need_neg = smb2_need_neg, .negotiate = smb2_negotiate, @@ -2589,6 +2592,7 @@ struct smb_version_operations smb311_operations = { .print_stats = smb2_print_stats, .dump_share_caps = smb2_dump_share_caps, .is_oplock_break = smb2_is_valid_oplock_break, + .handle_cancelled_mid = smb2_handle_cancelled_mid, .downgrade_oplock = smb2_downgrade_oplock, .need_neg = smb2_need_neg, .negotiate = smb2_negotiate, diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h index 69e3587..6853454 100644 --- a/fs/cifs/smb2proto.h +++ b/fs/cifs/smb2proto.h @@ -48,6 +48,10 @@ extern struct mid_q_entry *smb2_setup_request(struct cifs_ses *ses, struct smb_rqst *rqst); extern struct mid_q_entry *smb2_setup_async_request( struct TCP_Server_Info *server, struct smb_rqst *rqst); +extern struct cifs_ses *smb2_find_smb_ses(struct TCP_Server_Info *server, + __u64 ses_id); +extern struct cifs_tcon *smb2_find_smb_tcon(struct TCP_Server_Info *server, + __u64 ses_id, __u32 tid); extern int smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server); extern int smb3_calc_signature(struct smb_rqst *rqst, @@ -164,6 +168,9 @@ extern int SMB2_set_compression(const unsigned int xid, struct cifs_tcon *tcon, extern int SMB2_oplock_break(const unsigned int xid, struct cifs_tcon *tcon, const u64 persistent_fid, const u64 volatile_fid, const __u8 oplock_level); +extern int smb2_handle_cancelled_mid(char *buffer, + struct TCP_Server_Info *server); +void smb2_cancelled_close_fid(struct work_struct *work); extern int SMB2_QFS_info(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_file_id, u64 volatile_file_id, struct kstatfs *FSData); diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c index 7c3bb1b..e3c8a9c 100644 --- a/fs/cifs/smb2transport.c +++ b/fs/cifs/smb2transport.c @@ -115,23 +115,72 @@ smb3_crypto_shash_allocate(struct TCP_Server_Info *server) return 0; } -struct cifs_ses * -smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) +static struct cifs_ses * +_smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) { struct cifs_ses *ses; - spin_lock(&cifs_tcp_ses_lock); list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) { if (ses->Suid != ses_id) continue; - spin_unlock(&cifs_tcp_ses_lock); return ses; } + + return NULL; +} + +struct cifs_ses * +smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) +{ + struct cifs_ses *ses; + + spin_lock(&cifs_tcp_ses_lock); + ses = _smb2_find_smb_ses(server, ses_id); spin_unlock(&cifs_tcp_ses_lock); + return ses; +} + +static struct cifs_tcon * +_smb2_find_smb_sess_tcon(struct cifs_ses *ses, __u32 tid) +{ + struct list_head *tmp; + struct cifs_tcon *tcon; + + list_for_each(tmp, &ses->tcon_list) { + tcon = list_entry(tmp, struct cifs_tcon, tcon_list); + if (tcon->tid != tid) + continue; + ++tcon->tc_count; + return tcon; + } + return NULL; } +/* + * Obtain tcon corresponding to the tid in the given + * cifs_ses + */ + +struct cifs_tcon * +smb2_find_smb_tcon(struct TCP_Server_Info *server, __u64 ses_id, __u32 tid) +{ + struct cifs_ses *ses; + struct cifs_tcon *tcon; + + spin_lock(&cifs_tcp_ses_lock); + ses = _smb2_find_smb_ses(server, ses_id); + if (!ses) { + spin_unlock(&cifs_tcp_ses_lock); + return NULL; + } + tcon = _smb2_find_smb_sess_tcon(ses, tid); + spin_unlock(&cifs_tcp_ses_lock); + + return tcon; +} + int smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server) { @@ -511,6 +560,7 @@ smb2_mid_entry_alloc(const struct smb2_sync_hdr *shdr, atomic_inc(&midCount); temp->mid_state = MID_REQUEST_ALLOCATED; + temp->mid_flags = 0; return temp; } diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c index 526f053..d82a4ae 100644 --- a/fs/cifs/transport.c +++ b/fs/cifs/transport.c @@ -752,9 +752,12 @@ cifs_send_recv(const unsigned int xid, struct cifs_ses *ses, rc = wait_for_response(ses->server, midQ); if (rc != 0) { + cifs_dbg(FYI, "Cancelling wait for mid %lu\n", + (unsigned long)midQ->mid); send_cancel(ses->server, rqst, midQ); spin_lock(&GlobalMid_Lock); if (midQ->mid_state == MID_REQUEST_SUBMITTED) { + midQ->mid_flags |= MID_WAIT_CANCELLED; midQ->callback = DeleteMidQEntry; spin_unlock(&GlobalMid_Lock); add_credits(ses->server, 1, optype); -- 2.9.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
[parent not found: <1491503047.8010.2.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]
* Re: [bug report] Handle mismatched open calls [not found] ` <1491503047.8010.2.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> @ 2017-04-07 1:28 ` Steve French 2017-04-07 8:20 ` Dan Carpenter 1 sibling, 0 replies; 8+ messages in thread From: Steve French @ 2017-04-07 1:28 UTC (permalink / raw) To: Sachin Prabhu Cc: Dan Carpenter, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org merged into cifs-2-6.git - let me know if you see any problems with these ASAP On Thu, Apr 6, 2017 at 1:24 PM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > On Thu, 2017-04-06 at 11:07 +0100, Sachin Prabhu wrote: >> On Thu, 2017-04-06 at 12:11 +0300, Dan Carpenter wrote: >> > Hello Sachin Prabhu, >> > >> > The patch 96b75d0508f8: "Handle mismatched open calls" from Mar 3, >> > 2017, leads to the following static checker warning: >> > >> > fs/cifs/cifssmb.c:1530 cifs_readv_receive() >> > error: potential NULL dereference 'server->smallbuf'. >> > >> > fs/cifs/cifssmb.c >> > 1519 cifs_dbg(FYI, "0: iov_base=%p iov_len=%u\n", >> > 1520 rdata->iov[0].iov_base, server- >> > >total_read); >> > 1521 >> > 1522 mid->resp_buf = server->smallbuf; >> > 1523 server->smallbuf = NULL; >> > ^^^^^^^^^^^^^^^^^^^^^^^ >> > We set this to NULL here >> > >> > 1524 >> > 1525 /* how much data is in the response? */ >> > 1526 data_len = server->ops->read_data_length(buf); >> > 1527 if (data_offset + data_len > buflen) { >> > 1528 /* data_len is corrupt -- discard frame */ >> > 1529 rdata->result = -EIO; >> > 1530 return cifs_readv_discard(server, mid); >> > ^^^^^^ >> > but we need it here. >> > >> > 1531 } >> > 1532 >> > 1533 length = rdata->read_into_pages(server, rdata, >> > data_len); >> > 1534 if (length < 0) >> > 1535 return length; >> > >> > regards, >> > dan carpenter >> >> Thanks Dan, >> > > Hello Dan, > > I have attached a patch which replaces the original > "Handle mismatched open calls" patch from the for-next branch. > Can you please run it against the static checker. > > In the patch, we pass a buffer to cifs_discard_remaining_data() instead > of using server->small_buf to obtain the rfc len. > > Thanks > Sachin Prabhu -- Thanks, Steve ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] Handle mismatched open calls [not found] ` <1491503047.8010.2.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2017-04-07 1:28 ` Steve French @ 2017-04-07 8:20 ` Dan Carpenter 2017-04-07 12:18 ` Sachin Prabhu 1 sibling, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2017-04-07 8:20 UTC (permalink / raw) To: Sachin Prabhu; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA, smfrench Sure, that takes care of the static checker warning. But then I had all sorts of style nits to whinge-bucket about which you didn't ask for... On Thu, Apr 06, 2017 at 07:24:07PM +0100, Sachin Prabhu wrote: > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 9ae695a..131663b 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -904,10 +904,19 @@ cifs_demultiplex_thread(void *p) > > server->lstrp = jiffies; > if (mid_entry != NULL) { > + if ((mid_entry->mid_flags & MID_WAIT_CANCELLED) && > + mid_entry->mid_state == MID_RESPONSE_RECEIVED && > + (server->ops->handle_cancelled_mid)) Extra parens are not required. Please align it like this: if ((mid_entry->mid_flags & MID_WAIT_CANCELLED) && mid_entry->mid_state == MID_RESPONSE_RECEIVED && server->ops->handle_cancelled_mid) > + server->ops->handle_cancelled_mid( > + mid_entry->resp_buf, > + server); > + > if (!mid_entry->multiRsp || mid_entry->multiEnd) > mid_entry->callback(mid_entry); > - } else if (!server->ops->is_oplock_break || > - !server->ops->is_oplock_break(buf, server)) { > + } else if (server->ops->is_oplock_break && > + server->ops->is_oplock_break(buf, server)) { > + cifs_dbg(FYI, "Received oplock break\n"); > + } else { > cifs_dbg(VFS, "No task to wake, unknown frame received! NumMids %d\n", > atomic_read(&midCount)); > cifs_dump_mem("Received Data is: ", buf, > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c > index fd516ea..f50e3ef 100644 > --- a/fs/cifs/smb2misc.c > +++ b/fs/cifs/smb2misc.c > @@ -659,3 +659,51 @@ smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server) > cifs_dbg(FYI, "Can not process oplock break for non-existent connection\n"); > return false; > } > + > +void > +smb2_cancelled_close_fid(struct work_struct *work) > +{ > + struct close_cancelled_open *cancelled = container_of(work, > + struct close_cancelled_open, work); > + > + cifs_dbg(VFS, "Close unmatched open\n"); > + > + SMB2_close(0, cancelled->tcon, cancelled->fid.persistent_fid, > + cancelled->fid.volatile_fid); Don't right align it. Do it like this: SMB2_close(0, cancelled->tcon, cancelled->fid.persistent_fid, cancelled->fid.volatile_fid); > + cifs_put_tcon(cancelled->tcon); > + kfree(cancelled); > +} > + > +int > +smb2_handle_cancelled_mid(char *buffer, struct TCP_Server_Info *server) > +{ > + struct smb2_sync_hdr *sync_hdr = get_sync_hdr(buffer); > + struct smb2_create_rsp *rsp = (struct smb2_create_rsp *)buffer; > + struct cifs_tcon *tcon; > + struct close_cancelled_open *cancelled; > + > + if ((sync_hdr->Command != SMB2_CREATE) || > + (sync_hdr->Status != STATUS_SUCCESS)) No need for extra parens. Align it like this: if (sync_hdr->Command != SMB2_CREATE || sync_hdr->Status != STATUS_SUCCESS) return 0; > + return 0; > + > + cancelled = (struct close_cancelled_open *) > + kzalloc(sizeof(struct close_cancelled_open), > + GFP_KERNEL); No need to cast kzalloc. Use sizeof(*cancelled). cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL); > + if (!cancelled) > + return -ENOMEM; > + > + tcon = smb2_find_smb_tcon(server, sync_hdr->SessionId, > + sync_hdr->TreeId); > + if (!tcon) { > + kfree(cancelled); > + return -ENOENT; > + } > + > + cancelled->fid.persistent_fid = rsp->PersistentFileId; > + cancelled->fid.volatile_fid = rsp->VolatileFileId; > + cancelled->tcon = tcon; > + INIT_WORK(&cancelled->work, smb2_cancelled_close_fid); > + queue_work(cifsiod_wq, &cancelled->work); > + > + return 0; > +} [ snip ] > diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c > index 7c3bb1b..e3c8a9c 100644 > --- a/fs/cifs/smb2transport.c > +++ b/fs/cifs/smb2transport.c > @@ -115,23 +115,72 @@ smb3_crypto_shash_allocate(struct TCP_Server_Info *server) > return 0; > } > > -struct cifs_ses * > -smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) > +static struct cifs_ses * > +_smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) Could you name this "locked" or something and get rid of the _ underscore prefix? > { > struct cifs_ses *ses; > > - spin_lock(&cifs_tcp_ses_lock); > list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) { > if (ses->Suid != ses_id) > continue; > - spin_unlock(&cifs_tcp_ses_lock); > return ses; > } > + > + return NULL; > +} > + > +struct cifs_ses * > +smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) > +{ > + struct cifs_ses *ses; > + > + spin_lock(&cifs_tcp_ses_lock); > + ses = _smb2_find_smb_ses(server, ses_id); > spin_unlock(&cifs_tcp_ses_lock); > > + return ses; > +} > + > +static struct cifs_tcon * > +_smb2_find_smb_sess_tcon(struct cifs_ses *ses, __u32 tid) Just leave out the _ underscore prefix. This is a static function so there aren't that many users and they presumably know the locking rules. For smb2_find_smb_ses we have a locked and unlocked version of the function so it makes sense to put that in the name but here there is no need I think. > +{ > + struct list_head *tmp; > + struct cifs_tcon *tcon; > + > + list_for_each(tmp, &ses->tcon_list) { > + tcon = list_entry(tmp, struct cifs_tcon, tcon_list); list_for_each_entry()? > + if (tcon->tid != tid) > + continue; > + ++tcon->tc_count; > + return tcon; > + } > + > return NULL; > } > > +/* > + * Obtain tcon corresponding to the tid in the given > + * cifs_ses > + */ > + > +struct cifs_tcon * > +smb2_find_smb_tcon(struct TCP_Server_Info *server, __u64 ses_id, __u32 tid) > +{ > + struct cifs_ses *ses; > + struct cifs_tcon *tcon; > + > + spin_lock(&cifs_tcp_ses_lock); > + ses = _smb2_find_smb_ses(server, ses_id); > + if (!ses) { > + spin_unlock(&cifs_tcp_ses_lock); > + return NULL; > + } > + tcon = _smb2_find_smb_sess_tcon(ses, tid); > + spin_unlock(&cifs_tcp_ses_lock); > + > + return tcon; > +} > + > int > smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server) > { > @@ -511,6 +560,7 @@ smb2_mid_entry_alloc(const struct smb2_sync_hdr *shdr, > > atomic_inc(&midCount); > temp->mid_state = MID_REQUEST_ALLOCATED; > + temp->mid_flags = 0; No need. We already memset() this to zero. > return temp; > } > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > index 526f053..d82a4ae 100644 > --- a/fs/cifs/transport.c > +++ b/fs/cifs/transport.c > @@ -752,9 +752,12 @@ cifs_send_recv(const unsigned int xid, struct cifs_ses *ses, > > rc = wait_for_response(ses->server, midQ); > if (rc != 0) { > + cifs_dbg(FYI, "Cancelling wait for mid %lu\n", > + (unsigned long)midQ->mid); ->mid is u64. Just do this: cifs_dbg(FYI, "Cancelling wait for mid %llu\n", midQ->mid); > send_cancel(ses->server, rqst, midQ); > spin_lock(&GlobalMid_Lock); > if (midQ->mid_state == MID_REQUEST_SUBMITTED) { > + midQ->mid_flags |= MID_WAIT_CANCELLED; > midQ->callback = DeleteMidQEntry; > spin_unlock(&GlobalMid_Lock); > add_credits(ses->server, 1, optype); regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] Handle mismatched open calls 2017-04-07 8:20 ` Dan Carpenter @ 2017-04-07 12:18 ` Sachin Prabhu [not found] ` <1491567538.8010.6.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Sachin Prabhu @ 2017-04-07 12:18 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA, smfrench [-- Attachment #1: Type: text/plain, Size: 8320 bytes --] On Fri, 2017-04-07 at 11:20 +0300, Dan Carpenter wrote: > Sure, that takes care of the static checker warning. But then I had > all sorts of style nits to whinge-bucket about which you didn't ask > for... > Thanks Dan, I have incorporated all the requested changes into version 7 of the patch. Is smatch the only tool you use for static analysis? I am considering adding it to my own dev process. Steve, Can you please use this version instead. Regards, Sachin Prabhu > On Thu, Apr 06, 2017 at 07:24:07PM +0100, Sachin Prabhu wrote: > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > index 9ae695a..131663b 100644 > > --- a/fs/cifs/connect.c > > +++ b/fs/cifs/connect.c > > @@ -904,10 +904,19 @@ cifs_demultiplex_thread(void *p) > > > > server->lstrp = jiffies; > > if (mid_entry != NULL) { > > + if ((mid_entry->mid_flags & > > MID_WAIT_CANCELLED) && > > + mid_entry->mid_state == > > MID_RESPONSE_RECEIVED && > > + (server->ops- > > >handle_cancelled_mid)) > > Extra parens are not required. Please align it like this: > > if ((mid_entry->mid_flags & MID_WAIT_CANCELLED) > && > mid_entry->mid_state == > MID_RESPONSE_RECEIVED && > server->ops->handle_cancelled_mid) > > > > > + server->ops->handle_cancelled_mid( > > + mid_entry- > > >resp_buf, > > + server); > > + > > if (!mid_entry->multiRsp || mid_entry- > > >multiEnd) > > mid_entry->callback(mid_entry); > > - } else if (!server->ops->is_oplock_break || > > - !server->ops->is_oplock_break(buf, > > server)) { > > + } else if (server->ops->is_oplock_break && > > + server->ops->is_oplock_break(buf, > > server)) { > > + cifs_dbg(FYI, "Received oplock break\n"); > > + } else { > > cifs_dbg(VFS, "No task to wake, unknown > > frame received! NumMids %d\n", > > atomic_read(&midCount)); > > cifs_dump_mem("Received Data is: ", buf, > > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c > > index fd516ea..f50e3ef 100644 > > --- a/fs/cifs/smb2misc.c > > +++ b/fs/cifs/smb2misc.c > > @@ -659,3 +659,51 @@ smb2_is_valid_oplock_break(char *buffer, > > struct TCP_Server_Info *server) > > cifs_dbg(FYI, "Can not process oplock break for non- > > existent connection\n"); > > return false; > > } > > + > > +void > > +smb2_cancelled_close_fid(struct work_struct *work) > > +{ > > + struct close_cancelled_open *cancelled = > > container_of(work, > > + struct > > close_cancelled_open, work); > > + > > + cifs_dbg(VFS, "Close unmatched open\n"); > > + > > + SMB2_close(0, cancelled->tcon, cancelled- > > >fid.persistent_fid, > > + cancelled- > > >fid.volatile_fid); > > Don't right align it. Do it like this: > > SMB2_close(0, cancelled->tcon, cancelled->fid.persistent_fid, > cancelled->fid.volatile_fid); > > > > > + cifs_put_tcon(cancelled->tcon); > > + kfree(cancelled); > > +} > > + > > +int > > +smb2_handle_cancelled_mid(char *buffer, struct TCP_Server_Info > > *server) > > +{ > > + struct smb2_sync_hdr *sync_hdr = get_sync_hdr(buffer); > > + struct smb2_create_rsp *rsp = (struct smb2_create_rsp > > *)buffer; > > + struct cifs_tcon *tcon; > > + struct close_cancelled_open *cancelled; > > + > > + if ((sync_hdr->Command != SMB2_CREATE) || > > + (sync_hdr->Status != STATUS_SUCCESS)) > > No need for extra parens. Align it like this: > > if (sync_hdr->Command != SMB2_CREATE || > sync_hdr->Status != STATUS_SUCCESS) > return 0; > > > > > + return 0; > > + > > + cancelled = (struct close_cancelled_open *) > > + kzalloc(sizeof(struct > > close_cancelled_open), > > + GFP_KERNEL); > > No need to cast kzalloc. Use sizeof(*cancelled). > > cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL); > > > > + if (!cancelled) > > + return -ENOMEM; > > + > > + tcon = smb2_find_smb_tcon(server, sync_hdr->SessionId, > > + sync_hdr->TreeId); > > + if (!tcon) { > > + kfree(cancelled); > > + return -ENOENT; > > + } > > + > > + cancelled->fid.persistent_fid = rsp->PersistentFileId; > > + cancelled->fid.volatile_fid = rsp->VolatileFileId; > > + cancelled->tcon = tcon; > > + INIT_WORK(&cancelled->work, smb2_cancelled_close_fid); > > + queue_work(cifsiod_wq, &cancelled->work); > > + > > + return 0; > > +} > > [ snip ] > > > diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c > > index 7c3bb1b..e3c8a9c 100644 > > --- a/fs/cifs/smb2transport.c > > +++ b/fs/cifs/smb2transport.c > > @@ -115,23 +115,72 @@ smb3_crypto_shash_allocate(struct > > TCP_Server_Info *server) > > return 0; > > } > > > > -struct cifs_ses * > > -smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) > > +static struct cifs_ses * > > +_smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) > > Could you name this "locked" or something and get rid of the _ > underscore prefix? > > > { > > struct cifs_ses *ses; > > > > - spin_lock(&cifs_tcp_ses_lock); > > list_for_each_entry(ses, &server->smb_ses_list, > > smb_ses_list) { > > if (ses->Suid != ses_id) > > continue; > > - spin_unlock(&cifs_tcp_ses_lock); > > return ses; > > } > > + > > + return NULL; > > +} > > + > > +struct cifs_ses * > > +smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) > > +{ > > + struct cifs_ses *ses; > > + > > + spin_lock(&cifs_tcp_ses_lock); > > + ses = _smb2_find_smb_ses(server, ses_id); > > spin_unlock(&cifs_tcp_ses_lock); > > > > + return ses; > > +} > > + > > +static struct cifs_tcon * > > +_smb2_find_smb_sess_tcon(struct cifs_ses *ses, __u32 tid) > > Just leave out the _ underscore prefix. This is a static function so > there aren't that many users and they presumably know the locking > rules. > For smb2_find_smb_ses we have a locked and unlocked version of the > function so it makes sense to put that in the name but here there is > no need I think. > > > +{ > > + struct list_head *tmp; > > + struct cifs_tcon *tcon; > > + > > + list_for_each(tmp, &ses->tcon_list) { > > + tcon = list_entry(tmp, struct cifs_tcon, > > tcon_list); > > list_for_each_entry()? > > > + if (tcon->tid != tid) > > + continue; > > + ++tcon->tc_count; > > + return tcon; > > + } > > + > > return NULL; > > } > > > > +/* > > + * Obtain tcon corresponding to the tid in the given > > + * cifs_ses > > + */ > > + > > +struct cifs_tcon * > > +smb2_find_smb_tcon(struct TCP_Server_Info *server, __u64 ses_id, > > __u32 tid) > > +{ > > + struct cifs_ses *ses; > > + struct cifs_tcon *tcon; > > + > > + spin_lock(&cifs_tcp_ses_lock); > > + ses = _smb2_find_smb_ses(server, ses_id); > > + if (!ses) { > > + spin_unlock(&cifs_tcp_ses_lock); > > + return NULL; > > + } > > + tcon = _smb2_find_smb_sess_tcon(ses, tid); > > + spin_unlock(&cifs_tcp_ses_lock); > > + > > + return tcon; > > +} > > + > > int > > smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info > > *server) > > { > > @@ -511,6 +560,7 @@ smb2_mid_entry_alloc(const struct smb2_sync_hdr > > *shdr, > > > > atomic_inc(&midCount); > > temp->mid_state = MID_REQUEST_ALLOCATED; > > + temp->mid_flags = 0; > > No need. We already memset() this to zero. > > > return temp; > > } > > > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > > index 526f053..d82a4ae 100644 > > --- a/fs/cifs/transport.c > > +++ b/fs/cifs/transport.c > > @@ -752,9 +752,12 @@ cifs_send_recv(const unsigned int xid, struct > > cifs_ses *ses, > > > > rc = wait_for_response(ses->server, midQ); > > if (rc != 0) { > > + cifs_dbg(FYI, "Cancelling wait for mid %lu\n", > > + (unsigned long)midQ->mid); > > ->mid is u64. Just do this: > > cifs_dbg(FYI, "Cancelling wait for mid %llu\n", midQ- > >mid); > > > > send_cancel(ses->server, rqst, midQ); > > spin_lock(&GlobalMid_Lock); > > if (midQ->mid_state == MID_REQUEST_SUBMITTED) { > > + midQ->mid_flags |= MID_WAIT_CANCELLED; > > midQ->callback = DeleteMidQEntry; > > spin_unlock(&GlobalMid_Lock); > > add_credits(ses->server, 1, optype); > > regards, > dan carpenter > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" > in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at http://vger.kernel.org/majordomo-info.html [-- Attachment #2: 0001-v7-Handle-mismatched-open-calls.patch --] [-- Type: text/x-patch, Size: 13667 bytes --] From 198b9b9ae1b77d1d79cf72a947d0ee899baa34b4 Mon Sep 17 00:00:00 2001 From: Sachin Prabhu <sprabhu@redhat.com> Date: Fri, 3 Mar 2017 15:41:38 -0800 Subject: [PATCH] [v7]Handle mismatched open calls A signal can interrupt a SendReceive call which result in incoming responses to the call being ignored. This is a problem for calls such as open which results in the successful response being ignored. This results in an open file resource on the server. The patch looks into responses which were cancelled after being sent and in case of successful open closes the open fids. For this patch, the check is only done in SendReceive2() RH-bz: 1403319 Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> Cc: Stable <stable@vger.kernel.org> --- fs/cifs/cifsglob.h | 11 ++++++++++ fs/cifs/cifsproto.h | 3 ++- fs/cifs/cifssmb.c | 11 ++++++---- fs/cifs/connect.c | 13 ++++++++++-- fs/cifs/smb2misc.c | 46 +++++++++++++++++++++++++++++++++++++++++ fs/cifs/smb2ops.c | 8 +++++-- fs/cifs/smb2proto.h | 7 +++++++ fs/cifs/smb2transport.c | 55 +++++++++++++++++++++++++++++++++++++++++++++---- fs/cifs/transport.c | 2 ++ 9 files changed, 143 insertions(+), 13 deletions(-) diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index c565e0db..d07f13a 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -243,6 +243,7 @@ struct smb_version_operations { /* verify the message */ int (*check_message)(char *, unsigned int, struct TCP_Server_Info *); bool (*is_oplock_break)(char *, struct TCP_Server_Info *); + int (*handle_cancelled_mid)(char *, struct TCP_Server_Info *); void (*downgrade_oplock)(struct TCP_Server_Info *, struct cifsInodeInfo *, bool); /* process transaction2 response */ @@ -1344,6 +1345,7 @@ struct mid_q_entry { void *callback_data; /* general purpose pointer for callback */ void *resp_buf; /* pointer to received SMB header */ int mid_state; /* wish this were enum but can not pass to wait_event */ + unsigned int mid_flags; __le16 command; /* smb command code */ bool large_buf:1; /* if valid response, is pointer to large buf */ bool multiRsp:1; /* multiple trans2 responses for one request */ @@ -1351,6 +1353,12 @@ struct mid_q_entry { bool decrypted:1; /* decrypted entry */ }; +struct close_cancelled_open { + struct cifs_fid fid; + struct cifs_tcon *tcon; + struct work_struct work; +}; + /* Make code in transport.c a little cleaner by moving update of optional stats into function below */ #ifdef CONFIG_CIFS_STATS2 @@ -1482,6 +1490,9 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param, #define MID_RESPONSE_MALFORMED 0x10 #define MID_SHUTDOWN 0x20 +/* Flags */ +#define MID_WAIT_CANCELLED 1 /* Cancelled while waiting for response */ + /* Types of response buffer returned from SendReceive2 */ #define CIFS_NO_BUFFER 0 /* Response buffer not returned */ #define CIFS_SMALL_BUFFER 1 diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index 97e5d23..ec5e5e5 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -79,7 +79,8 @@ extern void cifs_delete_mid(struct mid_q_entry *mid); 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); -extern int cifs_discard_remaining_data(struct TCP_Server_Info *server); +extern int cifs_discard_remaining_data(struct TCP_Server_Info *server, + char *buf); extern int cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst, mid_receive_t *receive, mid_callback_t *callback, diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index 0669506..967b9263 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -1400,9 +1400,9 @@ CIFS_open(const unsigned int xid, struct cifs_open_parms *oparms, int *oplock, * current bigbuf. */ int -cifs_discard_remaining_data(struct TCP_Server_Info *server) +cifs_discard_remaining_data(struct TCP_Server_Info *server, char *buf) { - unsigned int rfclen = get_rfc1002_length(server->smallbuf); + unsigned int rfclen = get_rfc1002_length(buf); int remaining = rfclen + 4 - server->total_read; while (remaining > 0) { @@ -1426,7 +1426,7 @@ cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid) int length; struct cifs_readdata *rdata = mid->callback_data; - length = cifs_discard_remaining_data(server); + length = cifs_discard_remaining_data(server, mid->resp_buf); dequeue_mid(mid, rdata->result); return length; } @@ -1459,7 +1459,7 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid) if (server->ops->is_status_pending && server->ops->is_status_pending(buf, server, 0)) { - cifs_discard_remaining_data(server); + cifs_discard_remaining_data(server, buf); return -1; } @@ -1519,6 +1519,9 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid) cifs_dbg(FYI, "0: iov_base=%p iov_len=%u\n", rdata->iov[0].iov_base, server->total_read); + mid->resp_buf = server->smallbuf; + server->smallbuf = NULL; + /* how much data is in the response? */ data_len = server->ops->read_data_length(buf); if (data_offset + data_len > buflen) { diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 9ae695a..0c7596c 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -904,10 +904,19 @@ cifs_demultiplex_thread(void *p) server->lstrp = jiffies; if (mid_entry != NULL) { + if ((mid_entry->mid_flags & MID_WAIT_CANCELLED) && + mid_entry->mid_state == MID_RESPONSE_RECEIVED && + server->ops->handle_cancelled_mid) + server->ops->handle_cancelled_mid( + mid_entry->resp_buf, + server); + if (!mid_entry->multiRsp || mid_entry->multiEnd) mid_entry->callback(mid_entry); - } else if (!server->ops->is_oplock_break || - !server->ops->is_oplock_break(buf, server)) { + } else if (server->ops->is_oplock_break && + server->ops->is_oplock_break(buf, server)) { + cifs_dbg(FYI, "Received oplock break\n"); + } else { cifs_dbg(VFS, "No task to wake, unknown frame received! NumMids %d\n", atomic_read(&midCount)); cifs_dump_mem("Received Data is: ", buf, diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c index fd516ea..1a04b3a 100644 --- a/fs/cifs/smb2misc.c +++ b/fs/cifs/smb2misc.c @@ -659,3 +659,49 @@ smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server) cifs_dbg(FYI, "Can not process oplock break for non-existent connection\n"); return false; } + +void +smb2_cancelled_close_fid(struct work_struct *work) +{ + struct close_cancelled_open *cancelled = container_of(work, + struct close_cancelled_open, work); + + cifs_dbg(VFS, "Close unmatched open\n"); + + SMB2_close(0, cancelled->tcon, cancelled->fid.persistent_fid, + cancelled->fid.volatile_fid); + cifs_put_tcon(cancelled->tcon); + kfree(cancelled); +} + +int +smb2_handle_cancelled_mid(char *buffer, struct TCP_Server_Info *server) +{ + struct smb2_sync_hdr *sync_hdr = get_sync_hdr(buffer); + struct smb2_create_rsp *rsp = (struct smb2_create_rsp *)buffer; + struct cifs_tcon *tcon; + struct close_cancelled_open *cancelled; + + if (sync_hdr->Command != SMB2_CREATE || + sync_hdr->Status != STATUS_SUCCESS) + return 0; + + cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL); + if (!cancelled) + return -ENOMEM; + + tcon = smb2_find_smb_tcon(server, sync_hdr->SessionId, + sync_hdr->TreeId); + if (!tcon) { + kfree(cancelled); + return -ENOENT; + } + + cancelled->fid.persistent_fid = rsp->PersistentFileId; + cancelled->fid.volatile_fid = rsp->VolatileFileId; + cancelled->tcon = tcon; + INIT_WORK(&cancelled->work, smb2_cancelled_close_fid); + queue_work(cifsiod_wq, &cancelled->work); + + return 0; +} diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 583a0e2..7b12a72 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -2195,7 +2195,7 @@ receive_encrypted_read(struct TCP_Server_Info *server, struct mid_q_entry **mid) if (rc) goto free_pages; - rc = cifs_discard_remaining_data(server); + rc = cifs_discard_remaining_data(server, buf); if (rc) goto free_pages; @@ -2221,7 +2221,7 @@ receive_encrypted_read(struct TCP_Server_Info *server, struct mid_q_entry **mid) kfree(pages); return rc; discard_data: - cifs_discard_remaining_data(server); + cifs_discard_remaining_data(server, buf); goto free_pages; } @@ -2329,6 +2329,7 @@ struct smb_version_operations smb20_operations = { .clear_stats = smb2_clear_stats, .print_stats = smb2_print_stats, .is_oplock_break = smb2_is_valid_oplock_break, + .handle_cancelled_mid = smb2_handle_cancelled_mid, .downgrade_oplock = smb2_downgrade_oplock, .need_neg = smb2_need_neg, .negotiate = smb2_negotiate, @@ -2411,6 +2412,7 @@ struct smb_version_operations smb21_operations = { .clear_stats = smb2_clear_stats, .print_stats = smb2_print_stats, .is_oplock_break = smb2_is_valid_oplock_break, + .handle_cancelled_mid = smb2_handle_cancelled_mid, .downgrade_oplock = smb2_downgrade_oplock, .need_neg = smb2_need_neg, .negotiate = smb2_negotiate, @@ -2495,6 +2497,7 @@ struct smb_version_operations smb30_operations = { .print_stats = smb2_print_stats, .dump_share_caps = smb2_dump_share_caps, .is_oplock_break = smb2_is_valid_oplock_break, + .handle_cancelled_mid = smb2_handle_cancelled_mid, .downgrade_oplock = smb2_downgrade_oplock, .need_neg = smb2_need_neg, .negotiate = smb2_negotiate, @@ -2589,6 +2592,7 @@ struct smb_version_operations smb311_operations = { .print_stats = smb2_print_stats, .dump_share_caps = smb2_dump_share_caps, .is_oplock_break = smb2_is_valid_oplock_break, + .handle_cancelled_mid = smb2_handle_cancelled_mid, .downgrade_oplock = smb2_downgrade_oplock, .need_neg = smb2_need_neg, .negotiate = smb2_negotiate, diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h index 69e3587..6853454 100644 --- a/fs/cifs/smb2proto.h +++ b/fs/cifs/smb2proto.h @@ -48,6 +48,10 @@ extern struct mid_q_entry *smb2_setup_request(struct cifs_ses *ses, struct smb_rqst *rqst); extern struct mid_q_entry *smb2_setup_async_request( struct TCP_Server_Info *server, struct smb_rqst *rqst); +extern struct cifs_ses *smb2_find_smb_ses(struct TCP_Server_Info *server, + __u64 ses_id); +extern struct cifs_tcon *smb2_find_smb_tcon(struct TCP_Server_Info *server, + __u64 ses_id, __u32 tid); extern int smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server); extern int smb3_calc_signature(struct smb_rqst *rqst, @@ -164,6 +168,9 @@ extern int SMB2_set_compression(const unsigned int xid, struct cifs_tcon *tcon, extern int SMB2_oplock_break(const unsigned int xid, struct cifs_tcon *tcon, const u64 persistent_fid, const u64 volatile_fid, const __u8 oplock_level); +extern int smb2_handle_cancelled_mid(char *buffer, + struct TCP_Server_Info *server); +void smb2_cancelled_close_fid(struct work_struct *work); extern int SMB2_QFS_info(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_file_id, u64 volatile_file_id, struct kstatfs *FSData); diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c index 7c3bb1b..506b67f 100644 --- a/fs/cifs/smb2transport.c +++ b/fs/cifs/smb2transport.c @@ -115,23 +115,70 @@ smb3_crypto_shash_allocate(struct TCP_Server_Info *server) return 0; } -struct cifs_ses * -smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) +static struct cifs_ses * +smb2_find_smb_ses_unlocked(struct TCP_Server_Info *server, __u64 ses_id) { struct cifs_ses *ses; - spin_lock(&cifs_tcp_ses_lock); list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) { if (ses->Suid != ses_id) continue; - spin_unlock(&cifs_tcp_ses_lock); return ses; } + + return NULL; +} + +struct cifs_ses * +smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) +{ + struct cifs_ses *ses; + + spin_lock(&cifs_tcp_ses_lock); + ses = smb2_find_smb_ses_unlocked(server, ses_id); spin_unlock(&cifs_tcp_ses_lock); + return ses; +} + +static struct cifs_tcon * +smb2_find_smb_sess_tcon_unlocked(struct cifs_ses *ses, __u32 tid) +{ + struct cifs_tcon *tcon; + + list_for_each_entry(tcon, &ses->tcon_list, tcon_list) { + if (tcon->tid != tid) + continue; + ++tcon->tc_count; + return tcon; + } + return NULL; } +/* + * Obtain tcon corresponding to the tid in the given + * cifs_ses + */ + +struct cifs_tcon * +smb2_find_smb_tcon(struct TCP_Server_Info *server, __u64 ses_id, __u32 tid) +{ + struct cifs_ses *ses; + struct cifs_tcon *tcon; + + spin_lock(&cifs_tcp_ses_lock); + ses = smb2_find_smb_ses_unlocked(server, ses_id); + if (!ses) { + spin_unlock(&cifs_tcp_ses_lock); + return NULL; + } + tcon = smb2_find_smb_sess_tcon_unlocked(ses, tid); + spin_unlock(&cifs_tcp_ses_lock); + + return tcon; +} + int smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server) { diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c index 526f053..f6e13a9 100644 --- a/fs/cifs/transport.c +++ b/fs/cifs/transport.c @@ -752,9 +752,11 @@ cifs_send_recv(const unsigned int xid, struct cifs_ses *ses, rc = wait_for_response(ses->server, midQ); if (rc != 0) { + cifs_dbg(FYI, "Cancelling wait for mid %llu\n", midQ->mid); send_cancel(ses->server, rqst, midQ); spin_lock(&GlobalMid_Lock); if (midQ->mid_state == MID_REQUEST_SUBMITTED) { + midQ->mid_flags |= MID_WAIT_CANCELLED; midQ->callback = DeleteMidQEntry; spin_unlock(&GlobalMid_Lock); add_credits(ses->server, 1, optype); -- 2.9.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
[parent not found: <1491567538.8010.6.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]
* Re: [bug report] Handle mismatched open calls [not found] ` <1491567538.8010.6.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> @ 2017-04-07 13:05 ` Steve French [not found] ` <CAH2r5muHaA0JJ7E6fLFxO69wvLtuep8pC8_GdSsM_Lj2drYQOA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Steve French @ 2017-04-07 13:05 UTC (permalink / raw) To: Sachin Prabhu Cc: Dan Carpenter, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org merged into cifs-2.6.git for-next On Fri, Apr 7, 2017 at 7:18 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > On Fri, 2017-04-07 at 11:20 +0300, Dan Carpenter wrote: >> Sure, that takes care of the static checker warning. But then I had >> all sorts of style nits to whinge-bucket about which you didn't ask >> for... >> > > Thanks Dan, > > I have incorporated all the requested changes into version 7 of the > patch. > > Is smatch the only tool you use for static analysis? I am considering > adding it to my own dev process. > > Steve, Can you please use this version instead. > > Regards, > Sachin Prabhu > > >> On Thu, Apr 06, 2017 at 07:24:07PM +0100, Sachin Prabhu wrote: >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c >> > index 9ae695a..131663b 100644 >> > --- a/fs/cifs/connect.c >> > +++ b/fs/cifs/connect.c >> > @@ -904,10 +904,19 @@ cifs_demultiplex_thread(void *p) >> > >> > server->lstrp = jiffies; >> > if (mid_entry != NULL) { >> > + if ((mid_entry->mid_flags & >> > MID_WAIT_CANCELLED) && >> > + mid_entry->mid_state == >> > MID_RESPONSE_RECEIVED && >> > + (server->ops- >> > >handle_cancelled_mid)) >> >> Extra parens are not required. Please align it like this: >> >> if ((mid_entry->mid_flags & MID_WAIT_CANCELLED) >> && >> mid_entry->mid_state == >> MID_RESPONSE_RECEIVED && >> server->ops->handle_cancelled_mid) >> >> >> >> > + server->ops->handle_cancelled_mid( >> > + mid_entry- >> > >resp_buf, >> > + server); >> > + >> > if (!mid_entry->multiRsp || mid_entry- >> > >multiEnd) >> > mid_entry->callback(mid_entry); >> > - } else if (!server->ops->is_oplock_break || >> > - !server->ops->is_oplock_break(buf, >> > server)) { >> > + } else if (server->ops->is_oplock_break && >> > + server->ops->is_oplock_break(buf, >> > server)) { >> > + cifs_dbg(FYI, "Received oplock break\n"); >> > + } else { >> > cifs_dbg(VFS, "No task to wake, unknown >> > frame received! NumMids %d\n", >> > atomic_read(&midCount)); >> > cifs_dump_mem("Received Data is: ", buf, >> > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c >> > index fd516ea..f50e3ef 100644 >> > --- a/fs/cifs/smb2misc.c >> > +++ b/fs/cifs/smb2misc.c >> > @@ -659,3 +659,51 @@ smb2_is_valid_oplock_break(char *buffer, >> > struct TCP_Server_Info *server) >> > cifs_dbg(FYI, "Can not process oplock break for non- >> > existent connection\n"); >> > return false; >> > } >> > + >> > +void >> > +smb2_cancelled_close_fid(struct work_struct *work) >> > +{ >> > + struct close_cancelled_open *cancelled = >> > container_of(work, >> > + struct >> > close_cancelled_open, work); >> > + >> > + cifs_dbg(VFS, "Close unmatched open\n"); >> > + >> > + SMB2_close(0, cancelled->tcon, cancelled- >> > >fid.persistent_fid, >> > + cancelled- >> > >fid.volatile_fid); >> >> Don't right align it. Do it like this: >> >> SMB2_close(0, cancelled->tcon, cancelled->fid.persistent_fid, >> cancelled->fid.volatile_fid); >> >> >> >> > + cifs_put_tcon(cancelled->tcon); >> > + kfree(cancelled); >> > +} >> > + >> > +int >> > +smb2_handle_cancelled_mid(char *buffer, struct TCP_Server_Info >> > *server) >> > +{ >> > + struct smb2_sync_hdr *sync_hdr = get_sync_hdr(buffer); >> > + struct smb2_create_rsp *rsp = (struct smb2_create_rsp >> > *)buffer; >> > + struct cifs_tcon *tcon; >> > + struct close_cancelled_open *cancelled; >> > + >> > + if ((sync_hdr->Command != SMB2_CREATE) || >> > + (sync_hdr->Status != STATUS_SUCCESS)) >> >> No need for extra parens. Align it like this: >> >> if (sync_hdr->Command != SMB2_CREATE || >> sync_hdr->Status != STATUS_SUCCESS) >> return 0; >> >> >> >> > + return 0; >> > + >> > + cancelled = (struct close_cancelled_open *) >> > + kzalloc(sizeof(struct >> > close_cancelled_open), >> > + GFP_KERNEL); >> >> No need to cast kzalloc. Use sizeof(*cancelled). >> >> cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL); >> >> >> > + if (!cancelled) >> > + return -ENOMEM; >> > + >> > + tcon = smb2_find_smb_tcon(server, sync_hdr->SessionId, >> > + sync_hdr->TreeId); >> > + if (!tcon) { >> > + kfree(cancelled); >> > + return -ENOENT; >> > + } >> > + >> > + cancelled->fid.persistent_fid = rsp->PersistentFileId; >> > + cancelled->fid.volatile_fid = rsp->VolatileFileId; >> > + cancelled->tcon = tcon; >> > + INIT_WORK(&cancelled->work, smb2_cancelled_close_fid); >> > + queue_work(cifsiod_wq, &cancelled->work); >> > + >> > + return 0; >> > +} >> >> [ snip ] >> >> > diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c >> > index 7c3bb1b..e3c8a9c 100644 >> > --- a/fs/cifs/smb2transport.c >> > +++ b/fs/cifs/smb2transport.c >> > @@ -115,23 +115,72 @@ smb3_crypto_shash_allocate(struct >> > TCP_Server_Info *server) >> > return 0; >> > } >> > >> > -struct cifs_ses * >> > -smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) >> > +static struct cifs_ses * >> > +_smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) >> >> Could you name this "locked" or something and get rid of the _ >> underscore prefix? >> >> > { >> > struct cifs_ses *ses; >> > >> > - spin_lock(&cifs_tcp_ses_lock); >> > list_for_each_entry(ses, &server->smb_ses_list, >> > smb_ses_list) { >> > if (ses->Suid != ses_id) >> > continue; >> > - spin_unlock(&cifs_tcp_ses_lock); >> > return ses; >> > } >> > + >> > + return NULL; >> > +} >> > + >> > +struct cifs_ses * >> > +smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) >> > +{ >> > + struct cifs_ses *ses; >> > + >> > + spin_lock(&cifs_tcp_ses_lock); >> > + ses = _smb2_find_smb_ses(server, ses_id); >> > spin_unlock(&cifs_tcp_ses_lock); >> > >> > + return ses; >> > +} >> > + >> > +static struct cifs_tcon * >> > +_smb2_find_smb_sess_tcon(struct cifs_ses *ses, __u32 tid) >> >> Just leave out the _ underscore prefix. This is a static function so >> there aren't that many users and they presumably know the locking >> rules. >> For smb2_find_smb_ses we have a locked and unlocked version of the >> function so it makes sense to put that in the name but here there is >> no need I think. >> >> > +{ >> > + struct list_head *tmp; >> > + struct cifs_tcon *tcon; >> > + >> > + list_for_each(tmp, &ses->tcon_list) { >> > + tcon = list_entry(tmp, struct cifs_tcon, >> > tcon_list); >> >> list_for_each_entry()? >> >> > + if (tcon->tid != tid) >> > + continue; >> > + ++tcon->tc_count; >> > + return tcon; >> > + } >> > + >> > return NULL; >> > } >> > >> > +/* >> > + * Obtain tcon corresponding to the tid in the given >> > + * cifs_ses >> > + */ >> > + >> > +struct cifs_tcon * >> > +smb2_find_smb_tcon(struct TCP_Server_Info *server, __u64 ses_id, >> > __u32 tid) >> > +{ >> > + struct cifs_ses *ses; >> > + struct cifs_tcon *tcon; >> > + >> > + spin_lock(&cifs_tcp_ses_lock); >> > + ses = _smb2_find_smb_ses(server, ses_id); >> > + if (!ses) { >> > + spin_unlock(&cifs_tcp_ses_lock); >> > + return NULL; >> > + } >> > + tcon = _smb2_find_smb_sess_tcon(ses, tid); >> > + spin_unlock(&cifs_tcp_ses_lock); >> > + >> > + return tcon; >> > +} >> > + >> > int >> > smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info >> > *server) >> > { >> > @@ -511,6 +560,7 @@ smb2_mid_entry_alloc(const struct smb2_sync_hdr >> > *shdr, >> > >> > atomic_inc(&midCount); >> > temp->mid_state = MID_REQUEST_ALLOCATED; >> > + temp->mid_flags = 0; >> >> No need. We already memset() this to zero. >> >> > return temp; >> > } >> > >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c >> > index 526f053..d82a4ae 100644 >> > --- a/fs/cifs/transport.c >> > +++ b/fs/cifs/transport.c >> > @@ -752,9 +752,12 @@ cifs_send_recv(const unsigned int xid, struct >> > cifs_ses *ses, >> > >> > rc = wait_for_response(ses->server, midQ); >> > if (rc != 0) { >> > + cifs_dbg(FYI, "Cancelling wait for mid %lu\n", >> > + (unsigned long)midQ->mid); >> >> ->mid is u64. Just do this: >> >> cifs_dbg(FYI, "Cancelling wait for mid %llu\n", midQ- >> >mid); >> >> >> > send_cancel(ses->server, rqst, midQ); >> > spin_lock(&GlobalMid_Lock); >> > if (midQ->mid_state == MID_REQUEST_SUBMITTED) { >> > + midQ->mid_flags |= MID_WAIT_CANCELLED; >> > midQ->callback = DeleteMidQEntry; >> > spin_unlock(&GlobalMid_Lock); >> > add_credits(ses->server, 1, optype); >> >> regards, >> dan carpenter >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-cifs" >> in >> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- Thanks, Steve ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <CAH2r5muHaA0JJ7E6fLFxO69wvLtuep8pC8_GdSsM_Lj2drYQOA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [bug report] Handle mismatched open calls [not found] ` <CAH2r5muHaA0JJ7E6fLFxO69wvLtuep8pC8_GdSsM_Lj2drYQOA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2017-04-07 13:18 ` Steve French 0 siblings, 0 replies; 8+ messages in thread From: Steve French @ 2017-04-07 13:18 UTC (permalink / raw) To: Sachin Prabhu Cc: Dan Carpenter, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org git.samba.org is down at the moment - will repush when it comes back up On Fri, Apr 7, 2017 at 8:05 AM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > merged into cifs-2.6.git for-next > > On Fri, Apr 7, 2017 at 7:18 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: >> On Fri, 2017-04-07 at 11:20 +0300, Dan Carpenter wrote: >>> Sure, that takes care of the static checker warning. But then I had >>> all sorts of style nits to whinge-bucket about which you didn't ask >>> for... >>> >> >> Thanks Dan, >> >> I have incorporated all the requested changes into version 7 of the >> patch. >> >> Is smatch the only tool you use for static analysis? I am considering >> adding it to my own dev process. >> >> Steve, Can you please use this version instead. >> >> Regards, >> Sachin Prabhu >> >> >>> On Thu, Apr 06, 2017 at 07:24:07PM +0100, Sachin Prabhu wrote: >>> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c >>> > index 9ae695a..131663b 100644 >>> > --- a/fs/cifs/connect.c >>> > +++ b/fs/cifs/connect.c >>> > @@ -904,10 +904,19 @@ cifs_demultiplex_thread(void *p) >>> > >>> > server->lstrp = jiffies; >>> > if (mid_entry != NULL) { >>> > + if ((mid_entry->mid_flags & >>> > MID_WAIT_CANCELLED) && >>> > + mid_entry->mid_state == >>> > MID_RESPONSE_RECEIVED && >>> > + (server->ops- >>> > >handle_cancelled_mid)) >>> >>> Extra parens are not required. Please align it like this: >>> >>> if ((mid_entry->mid_flags & MID_WAIT_CANCELLED) >>> && >>> mid_entry->mid_state == >>> MID_RESPONSE_RECEIVED && >>> server->ops->handle_cancelled_mid) >>> >>> >>> >>> > + server->ops->handle_cancelled_mid( >>> > + mid_entry- >>> > >resp_buf, >>> > + server); >>> > + >>> > if (!mid_entry->multiRsp || mid_entry- >>> > >multiEnd) >>> > mid_entry->callback(mid_entry); >>> > - } else if (!server->ops->is_oplock_break || >>> > - !server->ops->is_oplock_break(buf, >>> > server)) { >>> > + } else if (server->ops->is_oplock_break && >>> > + server->ops->is_oplock_break(buf, >>> > server)) { >>> > + cifs_dbg(FYI, "Received oplock break\n"); >>> > + } else { >>> > cifs_dbg(VFS, "No task to wake, unknown >>> > frame received! NumMids %d\n", >>> > atomic_read(&midCount)); >>> > cifs_dump_mem("Received Data is: ", buf, >>> > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c >>> > index fd516ea..f50e3ef 100644 >>> > --- a/fs/cifs/smb2misc.c >>> > +++ b/fs/cifs/smb2misc.c >>> > @@ -659,3 +659,51 @@ smb2_is_valid_oplock_break(char *buffer, >>> > struct TCP_Server_Info *server) >>> > cifs_dbg(FYI, "Can not process oplock break for non- >>> > existent connection\n"); >>> > return false; >>> > } >>> > + >>> > +void >>> > +smb2_cancelled_close_fid(struct work_struct *work) >>> > +{ >>> > + struct close_cancelled_open *cancelled = >>> > container_of(work, >>> > + struct >>> > close_cancelled_open, work); >>> > + >>> > + cifs_dbg(VFS, "Close unmatched open\n"); >>> > + >>> > + SMB2_close(0, cancelled->tcon, cancelled- >>> > >fid.persistent_fid, >>> > + cancelled- >>> > >fid.volatile_fid); >>> >>> Don't right align it. Do it like this: >>> >>> SMB2_close(0, cancelled->tcon, cancelled->fid.persistent_fid, >>> cancelled->fid.volatile_fid); >>> >>> >>> >>> > + cifs_put_tcon(cancelled->tcon); >>> > + kfree(cancelled); >>> > +} >>> > + >>> > +int >>> > +smb2_handle_cancelled_mid(char *buffer, struct TCP_Server_Info >>> > *server) >>> > +{ >>> > + struct smb2_sync_hdr *sync_hdr = get_sync_hdr(buffer); >>> > + struct smb2_create_rsp *rsp = (struct smb2_create_rsp >>> > *)buffer; >>> > + struct cifs_tcon *tcon; >>> > + struct close_cancelled_open *cancelled; >>> > + >>> > + if ((sync_hdr->Command != SMB2_CREATE) || >>> > + (sync_hdr->Status != STATUS_SUCCESS)) >>> >>> No need for extra parens. Align it like this: >>> >>> if (sync_hdr->Command != SMB2_CREATE || >>> sync_hdr->Status != STATUS_SUCCESS) >>> return 0; >>> >>> >>> >>> > + return 0; >>> > + >>> > + cancelled = (struct close_cancelled_open *) >>> > + kzalloc(sizeof(struct >>> > close_cancelled_open), >>> > + GFP_KERNEL); >>> >>> No need to cast kzalloc. Use sizeof(*cancelled). >>> >>> cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL); >>> >>> >>> > + if (!cancelled) >>> > + return -ENOMEM; >>> > + >>> > + tcon = smb2_find_smb_tcon(server, sync_hdr->SessionId, >>> > + sync_hdr->TreeId); >>> > + if (!tcon) { >>> > + kfree(cancelled); >>> > + return -ENOENT; >>> > + } >>> > + >>> > + cancelled->fid.persistent_fid = rsp->PersistentFileId; >>> > + cancelled->fid.volatile_fid = rsp->VolatileFileId; >>> > + cancelled->tcon = tcon; >>> > + INIT_WORK(&cancelled->work, smb2_cancelled_close_fid); >>> > + queue_work(cifsiod_wq, &cancelled->work); >>> > + >>> > + return 0; >>> > +} >>> >>> [ snip ] >>> >>> > diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c >>> > index 7c3bb1b..e3c8a9c 100644 >>> > --- a/fs/cifs/smb2transport.c >>> > +++ b/fs/cifs/smb2transport.c >>> > @@ -115,23 +115,72 @@ smb3_crypto_shash_allocate(struct >>> > TCP_Server_Info *server) >>> > return 0; >>> > } >>> > >>> > -struct cifs_ses * >>> > -smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) >>> > +static struct cifs_ses * >>> > +_smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) >>> >>> Could you name this "locked" or something and get rid of the _ >>> underscore prefix? >>> >>> > { >>> > struct cifs_ses *ses; >>> > >>> > - spin_lock(&cifs_tcp_ses_lock); >>> > list_for_each_entry(ses, &server->smb_ses_list, >>> > smb_ses_list) { >>> > if (ses->Suid != ses_id) >>> > continue; >>> > - spin_unlock(&cifs_tcp_ses_lock); >>> > return ses; >>> > } >>> > + >>> > + return NULL; >>> > +} >>> > + >>> > +struct cifs_ses * >>> > +smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id) >>> > +{ >>> > + struct cifs_ses *ses; >>> > + >>> > + spin_lock(&cifs_tcp_ses_lock); >>> > + ses = _smb2_find_smb_ses(server, ses_id); >>> > spin_unlock(&cifs_tcp_ses_lock); >>> > >>> > + return ses; >>> > +} >>> > + >>> > +static struct cifs_tcon * >>> > +_smb2_find_smb_sess_tcon(struct cifs_ses *ses, __u32 tid) >>> >>> Just leave out the _ underscore prefix. This is a static function so >>> there aren't that many users and they presumably know the locking >>> rules. >>> For smb2_find_smb_ses we have a locked and unlocked version of the >>> function so it makes sense to put that in the name but here there is >>> no need I think. >>> >>> > +{ >>> > + struct list_head *tmp; >>> > + struct cifs_tcon *tcon; >>> > + >>> > + list_for_each(tmp, &ses->tcon_list) { >>> > + tcon = list_entry(tmp, struct cifs_tcon, >>> > tcon_list); >>> >>> list_for_each_entry()? >>> >>> > + if (tcon->tid != tid) >>> > + continue; >>> > + ++tcon->tc_count; >>> > + return tcon; >>> > + } >>> > + >>> > return NULL; >>> > } >>> > >>> > +/* >>> > + * Obtain tcon corresponding to the tid in the given >>> > + * cifs_ses >>> > + */ >>> > + >>> > +struct cifs_tcon * >>> > +smb2_find_smb_tcon(struct TCP_Server_Info *server, __u64 ses_id, >>> > __u32 tid) >>> > +{ >>> > + struct cifs_ses *ses; >>> > + struct cifs_tcon *tcon; >>> > + >>> > + spin_lock(&cifs_tcp_ses_lock); >>> > + ses = _smb2_find_smb_ses(server, ses_id); >>> > + if (!ses) { >>> > + spin_unlock(&cifs_tcp_ses_lock); >>> > + return NULL; >>> > + } >>> > + tcon = _smb2_find_smb_sess_tcon(ses, tid); >>> > + spin_unlock(&cifs_tcp_ses_lock); >>> > + >>> > + return tcon; >>> > +} >>> > + >>> > int >>> > smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info >>> > *server) >>> > { >>> > @@ -511,6 +560,7 @@ smb2_mid_entry_alloc(const struct smb2_sync_hdr >>> > *shdr, >>> > >>> > atomic_inc(&midCount); >>> > temp->mid_state = MID_REQUEST_ALLOCATED; >>> > + temp->mid_flags = 0; >>> >>> No need. We already memset() this to zero. >>> >>> > return temp; >>> > } >>> > >>> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c >>> > index 526f053..d82a4ae 100644 >>> > --- a/fs/cifs/transport.c >>> > +++ b/fs/cifs/transport.c >>> > @@ -752,9 +752,12 @@ cifs_send_recv(const unsigned int xid, struct >>> > cifs_ses *ses, >>> > >>> > rc = wait_for_response(ses->server, midQ); >>> > if (rc != 0) { >>> > + cifs_dbg(FYI, "Cancelling wait for mid %lu\n", >>> > + (unsigned long)midQ->mid); >>> >>> ->mid is u64. Just do this: >>> >>> cifs_dbg(FYI, "Cancelling wait for mid %llu\n", midQ- >>> >mid); >>> >>> >>> > send_cancel(ses->server, rqst, midQ); >>> > spin_lock(&GlobalMid_Lock); >>> > if (midQ->mid_state == MID_REQUEST_SUBMITTED) { >>> > + midQ->mid_flags |= MID_WAIT_CANCELLED; >>> > midQ->callback = DeleteMidQEntry; >>> > spin_unlock(&GlobalMid_Lock); >>> > add_credits(ses->server, 1, optype); >>> >>> regards, >>> dan carpenter >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" >>> in >>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Thanks, > > Steve -- Thanks, Steve ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-04-07 13:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-06 9:11 [bug report] Handle mismatched open calls Dan Carpenter
2017-04-06 10:07 ` Sachin Prabhu
[not found] ` <1491473227.3042.2.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2017-04-06 18:24 ` Sachin Prabhu
[not found] ` <1491503047.8010.2.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2017-04-07 1:28 ` Steve French
2017-04-07 8:20 ` Dan Carpenter
2017-04-07 12:18 ` Sachin Prabhu
[not found] ` <1491567538.8010.6.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2017-04-07 13:05 ` Steve French
[not found] ` <CAH2r5muHaA0JJ7E6fLFxO69wvLtuep8pC8_GdSsM_Lj2drYQOA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-04-07 13:18 ` Steve French
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.