* [PATCH] smb2: simplify mid handling/dequeueing code
@ 2022-07-19 17:31 Enzo Matsumiya
2022-07-19 17:31 ` [PATCH] smb2: small refactor in smb2_check_message() Enzo Matsumiya
2022-07-21 3:26 ` [PATCH] smb2: simplify mid handling/dequeueing code Steve French
0 siblings, 2 replies; 6+ messages in thread
From: Enzo Matsumiya @ 2022-07-19 17:31 UTC (permalink / raw)
To: linux-cifs; +Cc: smfrench, pc, ronniesahlberg, nspmangalore, Enzo Matsumiya
Mostly a code cleanup, aiming to simplify handle_mid(), dequeue_mid(),
and smb2_find_mid(), and their callers.
Also remove the @malformed parameter from those and their callers, since
the mid_state was already known beforehand.
Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
fs/cifs/cifsglob.h | 3 +-
fs/cifs/cifsproto.h | 2 +-
fs/cifs/cifssmb.c | 33 ++++-----
fs/cifs/connect.c | 46 ++++++------
fs/cifs/smb1ops.c | 12 ++--
fs/cifs/smb2misc.c | 18 +++--
fs/cifs/smb2ops.c | 168 +++++++++++++++++---------------------------
7 files changed, 118 insertions(+), 164 deletions(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index a643c84ff1e9..ae57ede51cd3 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -283,8 +283,7 @@ struct smb_version_operations {
struct cifsInodeInfo *cinode, __u32 oplock,
unsigned int epoch, bool *purge_cache);
/* process transaction2 response */
- bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *,
- char *, int);
+ bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *, char *);
/* check if we need to negotiate */
bool (*need_neg)(struct TCP_Server_Info *);
/* negotiate to the server */
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index d59aebefa71c..9e34ea9c7b2a 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -235,7 +235,7 @@ extern unsigned int setup_authusers_ACE(struct cifs_ace *pace);
extern unsigned int setup_special_mode_ACE(struct cifs_ace *pace, __u64 nmode);
extern unsigned int setup_special_user_owner_ACE(struct cifs_ace *pace);
-extern void dequeue_mid(struct mid_q_entry *mid, bool malformed);
+extern void dequeue_mid(struct mid_q_entry *mid);
extern int cifs_read_from_socket(struct TCP_Server_Info *server, char *buf,
unsigned int to_read);
extern ssize_t cifs_discard_from_socket(struct TCP_Server_Info *server,
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 6371b9eebdad..33513b4ee0b3 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -1406,26 +1406,17 @@ cifs_discard_remaining_data(struct TCP_Server_Info *server)
}
static int
-__cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid,
- bool malformed)
+cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
{
int length;
length = cifs_discard_remaining_data(server);
- dequeue_mid(mid, malformed);
+ dequeue_mid(mid);
mid->resp_buf = server->smallbuf;
server->smallbuf = NULL;
return length;
}
-static int
-cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
-{
- struct cifs_readdata *rdata = mid->callback_data;
-
- return __cifs_readv_discard(server, mid, rdata->result);
-}
-
int
cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
{
@@ -1483,7 +1474,8 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
cifs_dbg(FYI, "%s: server returned error %d\n",
__func__, rdata->result);
/* normal error on read response */
- return __cifs_readv_discard(server, mid, false);
+ mid->mid_state = MID_RESPONSE_RECEIVED;
+ return cifs_readv_discard(server, mid);
}
/* Is there enough to get to the rest of the READ_RSP header? */
@@ -1491,8 +1483,7 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
cifs_dbg(FYI, "%s: server returned short header. got=%u expected=%zu\n",
__func__, server->total_read,
server->vals->read_rsp_size);
- rdata->result = -EIO;
- return cifs_readv_discard(server, mid);
+ goto err_discard;
}
data_offset = server->ops->read_data_offset(buf) +
@@ -1510,8 +1501,7 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
/* data_offset is beyond the end of smallbuf */
cifs_dbg(FYI, "%s: data offset (%u) beyond end of smallbuf\n",
__func__, data_offset);
- rdata->result = -EIO;
- return cifs_readv_discard(server, mid);
+ goto err_discard;
}
cifs_dbg(FYI, "%s: total_read=%u data_offset=%u\n",
@@ -1534,8 +1524,7 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
data_len = server->ops->read_data_length(buf, use_rdma_mr);
if (!use_rdma_mr && (data_offset + data_len > buflen)) {
/* data_len is corrupt -- discard frame */
- rdata->result = -EIO;
- return cifs_readv_discard(server, mid);
+ goto err_discard;
}
length = rdata->read_into_pages(server, rdata, data_len);
@@ -1551,10 +1540,16 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
if (server->total_read < buflen)
return cifs_readv_discard(server, mid);
- dequeue_mid(mid, false);
+ mid->mid_state = MID_RESPONSE_RECEIVED;
+ dequeue_mid(mid);
mid->resp_buf = server->smallbuf;
server->smallbuf = NULL;
return length;
+
+err_discard:
+ rdata->result = -EIO;
+ mid->mid_state = MID_RESPONSE_MALFORMED;
+ return cifs_readv_discard(server, mid);
}
static void
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 057237c9cb30..dd15e14bd433 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -844,28 +844,20 @@ is_smb_response(struct TCP_Server_Info *server, unsigned char type)
}
void
-dequeue_mid(struct mid_q_entry *mid, bool malformed)
+dequeue_mid(struct mid_q_entry *mid)
{
#ifdef CONFIG_CIFS_STATS2
mid->when_received = jiffies;
#endif
- spin_lock(&GlobalMid_Lock);
- if (!malformed)
- mid->mid_state = MID_RESPONSE_RECEIVED;
- else
- mid->mid_state = MID_RESPONSE_MALFORMED;
- /*
- * Trying to handle/dequeue a mid after the send_recv()
- * function has finished processing it is a bug.
- */
if (mid->mid_flags & MID_DELETED) {
- spin_unlock(&GlobalMid_Lock);
pr_warn_once("trying to dequeue a deleted mid\n");
- } else {
- list_del_init(&mid->qhead);
- mid->mid_flags |= MID_DELETED;
- spin_unlock(&GlobalMid_Lock);
+ return;
}
+
+ spin_lock(&GlobalMid_Lock);
+ list_del_init(&mid->qhead);
+ mid->mid_flags |= MID_DELETED;
+ spin_unlock(&GlobalMid_Lock);
}
static unsigned int
@@ -883,15 +875,16 @@ smb2_get_credits_from_hdr(char *buffer, struct TCP_Server_Info *server)
}
static void
-handle_mid(struct mid_q_entry *mid, struct TCP_Server_Info *server,
- char *buf, int malformed)
+handle_mid(struct mid_q_entry *mid, struct TCP_Server_Info *server, char *buf)
{
if (server->ops->check_trans2 &&
- server->ops->check_trans2(mid, server, buf, malformed))
+ server->ops->check_trans2(mid, server, buf))
return;
+
mid->credits_received = smb2_get_credits_from_hdr(buf, server);
mid->resp_buf = buf;
mid->large_buf = server->large_buf;
+
/* Was previous buf put in mpx struct for multi-rsp? */
if (!mid->multiRsp) {
/* smb buffer will be freed by user thread */
@@ -900,7 +893,8 @@ handle_mid(struct mid_q_entry *mid, struct TCP_Server_Info *server,
else
server->smallbuf = NULL;
}
- dequeue_mid(mid, malformed);
+
+ dequeue_mid(mid);
}
static void clean_demultiplex_info(struct TCP_Server_Info *server)
@@ -1050,9 +1044,6 @@ cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
* into the payload for debugging purposes.
*/
rc = server->ops->check_message(buf, server->total_read, server);
- if (rc)
- cifs_dump_mem("Bad SMB: ", buf,
- min_t(unsigned int, server->total_read, 48));
if (server->ops->is_session_expired &&
server->ops->is_session_expired(buf)) {
@@ -1067,7 +1058,16 @@ cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
if (!mid)
return rc;
- handle_mid(mid, server, buf, rc);
+ if (unlikely(rc)) {
+ cifs_dump_mem("Bad SMB: ", buf,
+ min_t(unsigned int, server->total_read, 48));
+ /* mid is malformed */
+ mid->mid_state = MID_RESPONSE_MALFORMED;
+ } else {
+ mid->mid_state = MID_RESPONSE_RECEIVED;
+ }
+
+ handle_mid(mid, server, buf);
return 0;
}
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index 2e20ee4dab7b..416293fe14fb 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -384,21 +384,21 @@ cifs_downgrade_oplock(struct TCP_Server_Info *server,
static bool
cifs_check_trans2(struct mid_q_entry *mid, struct TCP_Server_Info *server,
- char *buf, int malformed)
+ char *buf)
{
- if (malformed)
- return false;
if (check2ndT2(buf) <= 0)
return false;
mid->multiRsp = true;
if (mid->resp_buf) {
+ int rc;
/* merge response - fix up 1st*/
- malformed = coalesce_t2(buf, mid->resp_buf);
- if (malformed > 0)
+ rc = coalesce_t2(buf, mid->resp_buf);
+ if (rc > 0)
return true;
/* All parts received or packet is malformed. */
mid->multiEnd = true;
- dequeue_mid(mid, malformed);
+ mid->mid_state = MID_RESPONSE_RECEIVED;
+ dequeue_mid(mid);
return true;
}
if (!server->large_buf) {
diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index 562064fe9668..0d57341e4f4a 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -26,17 +26,15 @@ check_smb2_hdr(struct smb2_hdr *shdr, __u64 mid)
* Make sure that this really is an SMB, that it is a response,
* and that the message ids match.
*/
- if ((shdr->ProtocolId == SMB2_PROTO_NUMBER) &&
- (mid == wire_mid)) {
+ if ((shdr->ProtocolId == SMB2_PROTO_NUMBER) && (mid == wire_mid)) {
if (shdr->Flags & SMB2_FLAGS_SERVER_TO_REDIR)
return 0;
- else {
- /* only one valid case where server sends us request */
- if (shdr->Command == SMB2_OPLOCK_BREAK)
- return 0;
- else
- cifs_dbg(VFS, "Received Request not response\n");
- }
+
+ /* only one valid case where server sends us request */
+ if (shdr->Command == SMB2_OPLOCK_BREAK)
+ return 0;
+
+ cifs_dbg(VFS, "Received Request not response\n");
} else { /* bad signature or mid */
if (shdr->ProtocolId != SMB2_PROTO_NUMBER)
cifs_dbg(VFS, "Bad protocol string signature header %x\n",
@@ -45,7 +43,7 @@ check_smb2_hdr(struct smb2_hdr *shdr, __u64 mid)
cifs_dbg(VFS, "Mids do not match: %llu and %llu\n",
mid, wire_mid);
}
- cifs_dbg(VFS, "Bad SMB detected. The Mid=%llu\n", wire_mid);
+ cifs_dbg(VFS, "Bad SMB detected, mid=%llu\n", wire_mid);
return 1;
}
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 8802995b2d3d..f63139015afa 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -335,7 +335,7 @@ smb2_revert_current_mid(struct TCP_Server_Info *server, const unsigned int val)
}
static struct mid_q_entry *
-__smb2_find_mid(struct TCP_Server_Info *server, char *buf, bool dequeue)
+smb2_find_mid(struct TCP_Server_Info *server, char *buf)
{
struct mid_q_entry *mid;
struct smb2_hdr *shdr = (struct smb2_hdr *)buf;
@@ -352,10 +352,6 @@ __smb2_find_mid(struct TCP_Server_Info *server, char *buf, bool dequeue)
(mid->mid_state == MID_REQUEST_SUBMITTED) &&
(mid->command == shdr->Command)) {
kref_get(&mid->refcount);
- if (dequeue) {
- list_del_init(&mid->qhead);
- mid->mid_flags |= MID_DELETED;
- }
spin_unlock(&GlobalMid_Lock);
return mid;
}
@@ -364,18 +360,6 @@ __smb2_find_mid(struct TCP_Server_Info *server, char *buf, bool dequeue)
return NULL;
}
-static struct mid_q_entry *
-smb2_find_mid(struct TCP_Server_Info *server, char *buf)
-{
- return __smb2_find_mid(server, buf, false);
-}
-
-static struct mid_q_entry *
-smb2_find_dequeue_mid(struct TCP_Server_Info *server, char *buf)
-{
- return __smb2_find_mid(server, buf, true);
-}
-
static void
smb2_dump_detail(void *buf, struct TCP_Server_Info *server)
{
@@ -4912,7 +4896,7 @@ handle_read_data(struct TCP_Server_Info *server, struct mid_q_entry *mid,
}
if (server->ops->is_status_pending &&
- server->ops->is_status_pending(buf, server))
+ server->ops->is_status_pending(buf, server))
return -1;
/* set up first two iov to get credits */
@@ -4931,11 +4915,9 @@ handle_read_data(struct TCP_Server_Info *server, struct mid_q_entry *mid,
cifs_dbg(FYI, "%s: server returned error %d\n",
__func__, rdata->result);
/* normal error on read response */
- if (is_offloaded)
- mid->mid_state = MID_RESPONSE_RECEIVED;
- else
- dequeue_mid(mid, false);
- return 0;
+ mid->mid_state = MID_RESPONSE_RECEIVED;
+ length = 0;
+ goto err_out;
}
data_offset = server->ops->read_data_offset(buf);
@@ -4958,11 +4940,7 @@ handle_read_data(struct TCP_Server_Info *server, struct mid_q_entry *mid,
cifs_dbg(FYI, "%s: data offset (%u) beyond end of smallbuf\n",
__func__, data_offset);
rdata->result = -EIO;
- if (is_offloaded)
- mid->mid_state = MID_RESPONSE_MALFORMED;
- else
- dequeue_mid(mid, rdata->result);
- return 0;
+ goto err_malformed;
}
pad_len = data_offset - server->vals->read_rsp_size;
@@ -4977,32 +4955,19 @@ handle_read_data(struct TCP_Server_Info *server, struct mid_q_entry *mid,
cifs_dbg(FYI, "%s: data offset (%u) beyond 1st page of response\n",
__func__, data_offset);
rdata->result = -EIO;
- if (is_offloaded)
- mid->mid_state = MID_RESPONSE_MALFORMED;
- else
- dequeue_mid(mid, rdata->result);
- return 0;
+ goto err_malformed;
}
if (data_len > page_data_size - pad_len) {
/* data_len is corrupt -- discard frame */
rdata->result = -EIO;
- if (is_offloaded)
- mid->mid_state = MID_RESPONSE_MALFORMED;
- else
- dequeue_mid(mid, rdata->result);
- return 0;
+ goto err_malformed;
}
rdata->result = init_read_bvec(pages, npages, page_data_size,
cur_off, &bvec);
- if (rdata->result != 0) {
- if (is_offloaded)
- mid->mid_state = MID_RESPONSE_MALFORMED;
- else
- dequeue_mid(mid, rdata->result);
- return 0;
- }
+ if (rdata->result != 0)
+ goto err_malformed;
iov_iter_bvec(&iter, WRITE, bvec, npages, data_len);
} else if (buf_len >= data_offset + data_len) {
@@ -5015,24 +4980,26 @@ handle_read_data(struct TCP_Server_Info *server, struct mid_q_entry *mid,
/* read response payload cannot be in both buf and pages */
WARN_ONCE(1, "buf can not contain only a part of read data");
rdata->result = -EIO;
- if (is_offloaded)
- mid->mid_state = MID_RESPONSE_MALFORMED;
- else
- dequeue_mid(mid, rdata->result);
- return 0;
+ goto err_malformed;
}
length = rdata->copy_into_pages(server, rdata, &iter);
kfree(bvec);
- if (length < 0)
+err_out:
+ if (length <= 0)
return length;
- if (is_offloaded)
- mid->mid_state = MID_RESPONSE_RECEIVED;
+err_malformed:
+ if (rdata->result != 0)
+ mid->mid_state = MID_RESPONSE_MALFORMED;
else
- dequeue_mid(mid, false);
+ mid->mid_state = MID_RESPONSE_RECEIVED;
+
+ if (!is_offloaded)
+ dequeue_mid(mid);
+
return length;
}
@@ -5061,44 +5028,45 @@ static void smb2_decrypt_offload(struct work_struct *work)
}
dw->server->lstrp = jiffies;
- mid = smb2_find_dequeue_mid(dw->server, dw->buf);
- if (mid == NULL)
+ mid = smb2_find_mid(dw->server, dw->buf);
+ if (mid == NULL) {
cifs_dbg(FYI, "mid not found\n");
- else {
- mid->decrypted = true;
- rc = handle_read_data(dw->server, mid, dw->buf,
- dw->server->vals->read_rsp_size,
- dw->ppages, dw->npages, dw->len,
- true);
- if (rc >= 0) {
+ goto free_pages;
+ }
+
+ dequeue_mid(mid);
+
+ mid->decrypted = true;
+ rc = handle_read_data(dw->server, mid, dw->buf,
+ dw->server->vals->read_rsp_size, dw->ppages,
+ dw->npages, dw->len, true);
+ if (rc >= 0) {
#ifdef CONFIG_CIFS_STATS2
- mid->when_received = jiffies;
+ mid->when_received = jiffies;
#endif
- if (dw->server->ops->is_network_name_deleted)
- dw->server->ops->is_network_name_deleted(dw->buf,
- dw->server);
+ if (dw->server->ops->is_network_name_deleted)
+ dw->server->ops->is_network_name_deleted(dw->buf, dw->server);
- mid->callback(mid);
+ mid->callback(mid);
+ } else {
+ spin_lock(&cifs_tcp_ses_lock);
+ spin_lock(&GlobalMid_Lock);
+ if (dw->server->tcpStatus == CifsNeedReconnect) {
+ mid->mid_state = MID_RETRY_NEEDED;
} else {
- spin_lock(&cifs_tcp_ses_lock);
- spin_lock(&GlobalMid_Lock);
- if (dw->server->tcpStatus == CifsNeedReconnect) {
- mid->mid_state = MID_RETRY_NEEDED;
- spin_unlock(&GlobalMid_Lock);
- spin_unlock(&cifs_tcp_ses_lock);
- mid->callback(mid);
- } else {
- mid->mid_state = MID_REQUEST_SUBMITTED;
- mid->mid_flags &= ~(MID_DELETED);
- list_add_tail(&mid->qhead,
- &dw->server->pending_mid_q);
- spin_unlock(&GlobalMid_Lock);
- spin_unlock(&cifs_tcp_ses_lock);
- }
+ mid->mid_state = MID_REQUEST_SUBMITTED;
+ mid->mid_flags &= ~(MID_DELETED);
+ list_add_tail(&mid->qhead, &dw->server->pending_mid_q);
}
- cifs_mid_q_entry_release(mid);
+ spin_unlock(&GlobalMid_Lock);
+ spin_unlock(&cifs_tcp_ses_lock);
+
+ if (mid->mid_state == MID_RETRY_NEEDED)
+ mid->callback(mid);
}
+ cifs_mid_q_entry_release(mid);
+
free_pages:
for (i = dw->npages-1; i >= 0; i--)
put_page(dw->ppages[i]);
@@ -5191,22 +5159,18 @@ receive_encrypted_read(struct TCP_Server_Info *server, struct mid_q_entry **mid,
goto free_pages;
*mid = smb2_find_mid(server, buf);
- if (*mid == NULL)
+ if (*mid == NULL) {
cifs_dbg(FYI, "mid not found\n");
- else {
- cifs_dbg(FYI, "mid found\n");
- (*mid)->decrypted = true;
- rc = handle_read_data(server, *mid, buf,
- server->vals->read_rsp_size,
- pages, npages, len, false);
- if (rc >= 0) {
- if (server->ops->is_network_name_deleted) {
- server->ops->is_network_name_deleted(buf,
- server);
- }
- }
+ goto free_pages;
}
+ (*mid)->decrypted = true;
+ rc = handle_read_data(server, *mid, buf, server->vals->read_rsp_size,
+ pages, npages, len, false);
+ if (rc >= 0)
+ if (server->ops->is_network_name_deleted)
+ server->ops->is_network_name_deleted(buf, server);
+
free_pages:
for (i = i - 1; i >= 0; i--)
put_page(pages[i]);
@@ -5253,7 +5217,7 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
return length;
next_is_large = server->large_buf;
-one_more:
+next:
shdr = (struct smb2_hdr *)buf;
if (shdr->NextCommand) {
if (next_is_large)
@@ -5266,10 +5230,7 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
}
mid_entry = smb2_find_mid(server, buf);
- if (mid_entry == NULL)
- cifs_dbg(FYI, "mid not found\n");
- else {
- cifs_dbg(FYI, "mid found\n");
+ if (mid_entry) {
mid_entry->decrypted = true;
mid_entry->resp_buf_size = server->pdu_size;
}
@@ -5278,6 +5239,7 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
cifs_server_dbg(VFS, "too many PDUs in compound\n");
return -1;
}
+
bufs[*num_mids] = buf;
mids[(*num_mids)++] = mid_entry;
@@ -5293,7 +5255,7 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
server->bigbuf = buf = next_buffer;
else
server->smallbuf = buf = next_buffer;
- goto one_more;
+ goto next;
} else if (ret != 0) {
/*
* ret != 0 here means that we didn't get to handle_mid() thus
--
2.35.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] smb2: small refactor in smb2_check_message()
2022-07-19 17:31 [PATCH] smb2: simplify mid handling/dequeueing code Enzo Matsumiya
@ 2022-07-19 17:31 ` Enzo Matsumiya
2022-07-21 3:05 ` Steve French
2022-07-21 3:26 ` [PATCH] smb2: simplify mid handling/dequeueing code Steve French
1 sibling, 1 reply; 6+ messages in thread
From: Enzo Matsumiya @ 2022-07-19 17:31 UTC (permalink / raw)
To: linux-cifs; +Cc: smfrench, pc, ronniesahlberg, nspmangalore, Enzo Matsumiya
If the command is SMB2_IOCTL, OutputLength and OutputContext are
optional and can be zero, so return early and skip calculated length
check.
Move the mismatched length message to the end of the check, to avoid
unnecessary logs when the check was not a real miscalculation.
Also change the pr_warn_once() to a pr_warn() so we're sure to get a
log for the real mismatches.
Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
fs/cifs/connect.c | 13 ++++++-------
fs/cifs/smb2misc.c | 47 +++++++++++++++++++++++++++-------------------
2 files changed, 34 insertions(+), 26 deletions(-)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 386bb523c69e..057237c9cb30 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1039,19 +1039,18 @@ int
cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
{
char *buf = server->large_buf ? server->bigbuf : server->smallbuf;
- int length;
+ int rc;
/*
* We know that we received enough to get to the MID as we
* checked the pdu_length earlier. Now check to see
- * if the rest of the header is OK. We borrow the length
- * var for the rest of the loop to avoid a new stack var.
+ * if the rest of the header is OK.
*
* 48 bytes is enough to display the header and a little bit
* into the payload for debugging purposes.
*/
- length = server->ops->check_message(buf, server->total_read, server);
- if (length != 0)
+ rc = server->ops->check_message(buf, server->total_read, server);
+ if (rc)
cifs_dump_mem("Bad SMB: ", buf,
min_t(unsigned int, server->total_read, 48));
@@ -1066,9 +1065,9 @@ cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
return -1;
if (!mid)
- return length;
+ return rc;
- handle_mid(mid, server, buf, length);
+ handle_mid(mid, server, buf, rc);
return 0;
}
diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index 17813c3d0c6e..562064fe9668 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -132,15 +132,15 @@ static __u32 get_neg_ctxt_len(struct smb2_hdr *hdr, __u32 len,
}
int
-smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
+smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *server)
{
struct smb2_hdr *shdr = (struct smb2_hdr *)buf;
struct smb2_pdu *pdu = (struct smb2_pdu *)shdr;
- __u64 mid;
- __u32 clc_len; /* calculated length */
- int command;
- int pdu_size = sizeof(struct smb2_pdu);
int hdr_size = sizeof(struct smb2_hdr);
+ int pdu_size = sizeof(struct smb2_pdu);
+ int command;
+ __u32 calc_len; /* calculated length */
+ __u64 mid;
/*
* Add function to do table lookup of StructureSize by command
@@ -154,7 +154,7 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
/* decrypt frame now that it is completely read in */
spin_lock(&cifs_tcp_ses_lock);
- list_for_each_entry(iter, &srvr->smb_ses_list, smb_ses_list) {
+ list_for_each_entry(iter, &server->smb_ses_list, smb_ses_list) {
if (iter->Suid == le64_to_cpu(thdr->SessionId)) {
ses = iter;
break;
@@ -221,30 +221,33 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
}
}
- clc_len = smb2_calc_size(buf, srvr);
+ calc_len = smb2_calc_size(buf, server);
+
+ /* For SMB2_IOCTL, OutputOffset and OutputLength are optional, so might
+ * be 0, and not a real miscalculation */
+ if (command == SMB2_IOCTL_HE && calc_len == 0)
+ return 0;
- if (shdr->Command == SMB2_NEGOTIATE)
- clc_len += get_neg_ctxt_len(shdr, len, clc_len);
+ if (command == SMB2_NEGOTIATE_HE)
+ calc_len += get_neg_ctxt_len(shdr, len, calc_len);
- if (len != clc_len) {
- cifs_dbg(FYI, "Calculated size %u length %u mismatch mid %llu\n",
- clc_len, len, mid);
+ if (len != calc_len) {
/* create failed on symlink */
if (command == SMB2_CREATE_HE &&
shdr->Status == STATUS_STOPPED_ON_SYMLINK)
return 0;
/* Windows 7 server returns 24 bytes more */
- if (clc_len + 24 == len && command == SMB2_OPLOCK_BREAK_HE)
+ if (calc_len + 24 == len && command == SMB2_OPLOCK_BREAK_HE)
return 0;
/* server can return one byte more due to implied bcc[0] */
- if (clc_len == len + 1)
+ if (calc_len == len + 1)
return 0;
/*
* Some windows servers (win2016) will pad also the final
* PDU in a compound to 8 bytes.
*/
- if (((clc_len + 7) & ~7) == len)
+ if (((calc_len + 7) & ~7) == len)
return 0;
/*
@@ -253,12 +256,18 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
* SMB2/SMB3 frame length (header + smb2 response specific data)
* Some windows servers also pad up to 8 bytes when compounding.
*/
- if (clc_len < len)
+ if (calc_len < len)
return 0;
- pr_warn_once(
- "srv rsp too short, len %d not %d. cmd:%d mid:%llu\n",
- len, clc_len, command, mid);
+ /* Only log a message if len was really miscalculated */
+ if (unlikely(cifsFYI))
+ cifs_dbg(FYI, "Server response too short: calculated "
+ "length %u doesn't match read length %u (cmd=%d, mid=%llu)\n",
+ calc_len, len, command, mid);
+ else
+ pr_warn("Server response too short: calculated length "
+ "%u doesn't match read length %u (cmd=%d, mid=%llu)\n",
+ calc_len, len, command, mid);
return 1;
}
--
2.35.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] smb2: small refactor in smb2_check_message()
2022-07-19 17:31 ` [PATCH] smb2: small refactor in smb2_check_message() Enzo Matsumiya
@ 2022-07-21 3:05 ` Steve French
2022-07-21 20:47 ` Enzo Matsumiya
0 siblings, 1 reply; 6+ messages in thread
From: Steve French @ 2022-07-21 3:05 UTC (permalink / raw)
To: Enzo Matsumiya; +Cc: CIFS, Paulo Alcantara, ronnie sahlberg, Shyam Prasad N
I don't have any objections to this but wondering what prompted the
patch? Did you see an error logged with ioctls? You mention:
"SMB2_IOCTL, OutputLength and OutputContext are optional and can be zero"
And did you want to change the pr_warn_once to a pr_warn on the
mismatch since you had a case where server was frequently returning
frame with bad length and you want to debug it?
I am a little worried that it could cause log spam if some server has
a bug in smb3 response length.
On Tue, Jul 19, 2022 at 12:32 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> If the command is SMB2_IOCTL, OutputLength and OutputContext are
> optional and can be zero, so return early and skip calculated length
> check.
>
> Move the mismatched length message to the end of the check, to avoid
> unnecessary logs when the check was not a real miscalculation.
>
> Also change the pr_warn_once() to a pr_warn() so we're sure to get a
> log for the real mismatches.
>
> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> ---
> fs/cifs/connect.c | 13 ++++++-------
> fs/cifs/smb2misc.c | 47 +++++++++++++++++++++++++++-------------------
> 2 files changed, 34 insertions(+), 26 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 386bb523c69e..057237c9cb30 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1039,19 +1039,18 @@ int
> cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> {
> char *buf = server->large_buf ? server->bigbuf : server->smallbuf;
> - int length;
> + int rc;
>
> /*
> * We know that we received enough to get to the MID as we
> * checked the pdu_length earlier. Now check to see
> - * if the rest of the header is OK. We borrow the length
> - * var for the rest of the loop to avoid a new stack var.
> + * if the rest of the header is OK.
> *
> * 48 bytes is enough to display the header and a little bit
> * into the payload for debugging purposes.
> */
> - length = server->ops->check_message(buf, server->total_read, server);
> - if (length != 0)
> + rc = server->ops->check_message(buf, server->total_read, server);
> + if (rc)
> cifs_dump_mem("Bad SMB: ", buf,
> min_t(unsigned int, server->total_read, 48));
>
> @@ -1066,9 +1065,9 @@ cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> return -1;
>
> if (!mid)
> - return length;
> + return rc;
>
> - handle_mid(mid, server, buf, length);
> + handle_mid(mid, server, buf, rc);
> return 0;
> }
>
> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> index 17813c3d0c6e..562064fe9668 100644
> --- a/fs/cifs/smb2misc.c
> +++ b/fs/cifs/smb2misc.c
> @@ -132,15 +132,15 @@ static __u32 get_neg_ctxt_len(struct smb2_hdr *hdr, __u32 len,
> }
>
> int
> -smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
> +smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *server)
> {
> struct smb2_hdr *shdr = (struct smb2_hdr *)buf;
> struct smb2_pdu *pdu = (struct smb2_pdu *)shdr;
> - __u64 mid;
> - __u32 clc_len; /* calculated length */
> - int command;
> - int pdu_size = sizeof(struct smb2_pdu);
> int hdr_size = sizeof(struct smb2_hdr);
> + int pdu_size = sizeof(struct smb2_pdu);
> + int command;
> + __u32 calc_len; /* calculated length */
> + __u64 mid;
>
> /*
> * Add function to do table lookup of StructureSize by command
> @@ -154,7 +154,7 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
>
> /* decrypt frame now that it is completely read in */
> spin_lock(&cifs_tcp_ses_lock);
> - list_for_each_entry(iter, &srvr->smb_ses_list, smb_ses_list) {
> + list_for_each_entry(iter, &server->smb_ses_list, smb_ses_list) {
> if (iter->Suid == le64_to_cpu(thdr->SessionId)) {
> ses = iter;
> break;
> @@ -221,30 +221,33 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
> }
> }
>
> - clc_len = smb2_calc_size(buf, srvr);
> + calc_len = smb2_calc_size(buf, server);
> +
> + /* For SMB2_IOCTL, OutputOffset and OutputLength are optional, so might
> + * be 0, and not a real miscalculation */
> + if (command == SMB2_IOCTL_HE && calc_len == 0)
> + return 0;
>
> - if (shdr->Command == SMB2_NEGOTIATE)
> - clc_len += get_neg_ctxt_len(shdr, len, clc_len);
> + if (command == SMB2_NEGOTIATE_HE)
> + calc_len += get_neg_ctxt_len(shdr, len, calc_len);
>
> - if (len != clc_len) {
> - cifs_dbg(FYI, "Calculated size %u length %u mismatch mid %llu\n",
> - clc_len, len, mid);
> + if (len != calc_len) {
> /* create failed on symlink */
> if (command == SMB2_CREATE_HE &&
> shdr->Status == STATUS_STOPPED_ON_SYMLINK)
> return 0;
> /* Windows 7 server returns 24 bytes more */
> - if (clc_len + 24 == len && command == SMB2_OPLOCK_BREAK_HE)
> + if (calc_len + 24 == len && command == SMB2_OPLOCK_BREAK_HE)
> return 0;
> /* server can return one byte more due to implied bcc[0] */
> - if (clc_len == len + 1)
> + if (calc_len == len + 1)
> return 0;
>
> /*
> * Some windows servers (win2016) will pad also the final
> * PDU in a compound to 8 bytes.
> */
> - if (((clc_len + 7) & ~7) == len)
> + if (((calc_len + 7) & ~7) == len)
> return 0;
>
> /*
> @@ -253,12 +256,18 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
> * SMB2/SMB3 frame length (header + smb2 response specific data)
> * Some windows servers also pad up to 8 bytes when compounding.
> */
> - if (clc_len < len)
> + if (calc_len < len)
> return 0;
>
> - pr_warn_once(
> - "srv rsp too short, len %d not %d. cmd:%d mid:%llu\n",
> - len, clc_len, command, mid);
> + /* Only log a message if len was really miscalculated */
> + if (unlikely(cifsFYI))
> + cifs_dbg(FYI, "Server response too short: calculated "
> + "length %u doesn't match read length %u (cmd=%d, mid=%llu)\n",
> + calc_len, len, command, mid);
> + else
> + pr_warn("Server response too short: calculated length "
> + "%u doesn't match read length %u (cmd=%d, mid=%llu)\n",
> + calc_len, len, command, mid);
>
> return 1;
> }
> --
> 2.35.3
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] smb2: simplify mid handling/dequeueing code
2022-07-19 17:31 [PATCH] smb2: simplify mid handling/dequeueing code Enzo Matsumiya
2022-07-19 17:31 ` [PATCH] smb2: small refactor in smb2_check_message() Enzo Matsumiya
@ 2022-07-21 3:26 ` Steve French
2022-07-21 21:02 ` Enzo Matsumiya
1 sibling, 1 reply; 6+ messages in thread
From: Steve French @ 2022-07-21 3:26 UTC (permalink / raw)
To: Enzo Matsumiya; +Cc: rohiths msft, CIFS
Can you give some context as to why only smb2_find_dequeue_mid()
set the "dequeue" flag to true and why it doesn't need to be
distinguished from the other find_mid cases?
I think Rohith wrote this - Rohith does the change make sense to you? Thoughts?
Also Enzo can you explain a little what the callers were who set
"malformed" (and why it now no longer needs to be used)?
On Tue, Jul 19, 2022 at 12:32 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> Mostly a code cleanup, aiming to simplify handle_mid(), dequeue_mid(),
> and smb2_find_mid(), and their callers.
>
> Also remove the @malformed parameter from those and their callers, since
> the mid_state was already known beforehand.
>
> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> ---
> fs/cifs/cifsglob.h | 3 +-
> fs/cifs/cifsproto.h | 2 +-
> fs/cifs/cifssmb.c | 33 ++++-----
> fs/cifs/connect.c | 46 ++++++------
> fs/cifs/smb1ops.c | 12 ++--
> fs/cifs/smb2misc.c | 18 +++--
> fs/cifs/smb2ops.c | 168 +++++++++++++++++---------------------------
> 7 files changed, 118 insertions(+), 164 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index a643c84ff1e9..ae57ede51cd3 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -283,8 +283,7 @@ struct smb_version_operations {
> struct cifsInodeInfo *cinode, __u32 oplock,
> unsigned int epoch, bool *purge_cache);
> /* process transaction2 response */
> - bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *,
> - char *, int);
> + bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *, char *);
> /* check if we need to negotiate */
> bool (*need_neg)(struct TCP_Server_Info *);
> /* negotiate to the server */
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index d59aebefa71c..9e34ea9c7b2a 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -235,7 +235,7 @@ extern unsigned int setup_authusers_ACE(struct cifs_ace *pace);
> extern unsigned int setup_special_mode_ACE(struct cifs_ace *pace, __u64 nmode);
> extern unsigned int setup_special_user_owner_ACE(struct cifs_ace *pace);
>
> -extern void dequeue_mid(struct mid_q_entry *mid, bool malformed);
> +extern void dequeue_mid(struct mid_q_entry *mid);
> extern int cifs_read_from_socket(struct TCP_Server_Info *server, char *buf,
> unsigned int to_read);
> extern ssize_t cifs_discard_from_socket(struct TCP_Server_Info *server,
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 6371b9eebdad..33513b4ee0b3 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -1406,26 +1406,17 @@ cifs_discard_remaining_data(struct TCP_Server_Info *server)
> }
>
> static int
> -__cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid,
> - bool malformed)
> +cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> {
> int length;
>
> length = cifs_discard_remaining_data(server);
> - dequeue_mid(mid, malformed);
> + dequeue_mid(mid);
> mid->resp_buf = server->smallbuf;
> server->smallbuf = NULL;
> return length;
> }
>
> -static int
> -cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> -{
> - struct cifs_readdata *rdata = mid->callback_data;
> -
> - return __cifs_readv_discard(server, mid, rdata->result);
> -}
> -
> int
> cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> {
> @@ -1483,7 +1474,8 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> cifs_dbg(FYI, "%s: server returned error %d\n",
> __func__, rdata->result);
> /* normal error on read response */
> - return __cifs_readv_discard(server, mid, false);
> + mid->mid_state = MID_RESPONSE_RECEIVED;
> + return cifs_readv_discard(server, mid);
> }
>
> /* Is there enough to get to the rest of the READ_RSP header? */
> @@ -1491,8 +1483,7 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> cifs_dbg(FYI, "%s: server returned short header. got=%u expected=%zu\n",
> __func__, server->total_read,
> server->vals->read_rsp_size);
> - rdata->result = -EIO;
> - return cifs_readv_discard(server, mid);
> + goto err_discard;
> }
>
> data_offset = server->ops->read_data_offset(buf) +
> @@ -1510,8 +1501,7 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> /* data_offset is beyond the end of smallbuf */
> cifs_dbg(FYI, "%s: data offset (%u) beyond end of smallbuf\n",
> __func__, data_offset);
> - rdata->result = -EIO;
> - return cifs_readv_discard(server, mid);
> + goto err_discard;
> }
>
> cifs_dbg(FYI, "%s: total_read=%u data_offset=%u\n",
> @@ -1534,8 +1524,7 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> data_len = server->ops->read_data_length(buf, use_rdma_mr);
> if (!use_rdma_mr && (data_offset + data_len > buflen)) {
> /* data_len is corrupt -- discard frame */
> - rdata->result = -EIO;
> - return cifs_readv_discard(server, mid);
> + goto err_discard;
> }
>
> length = rdata->read_into_pages(server, rdata, data_len);
> @@ -1551,10 +1540,16 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> if (server->total_read < buflen)
> return cifs_readv_discard(server, mid);
>
> - dequeue_mid(mid, false);
> + mid->mid_state = MID_RESPONSE_RECEIVED;
> + dequeue_mid(mid);
> mid->resp_buf = server->smallbuf;
> server->smallbuf = NULL;
> return length;
> +
> +err_discard:
> + rdata->result = -EIO;
> + mid->mid_state = MID_RESPONSE_MALFORMED;
> + return cifs_readv_discard(server, mid);
> }
>
> static void
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 057237c9cb30..dd15e14bd433 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -844,28 +844,20 @@ is_smb_response(struct TCP_Server_Info *server, unsigned char type)
> }
>
> void
> -dequeue_mid(struct mid_q_entry *mid, bool malformed)
> +dequeue_mid(struct mid_q_entry *mid)
> {
> #ifdef CONFIG_CIFS_STATS2
> mid->when_received = jiffies;
> #endif
> - spin_lock(&GlobalMid_Lock);
> - if (!malformed)
> - mid->mid_state = MID_RESPONSE_RECEIVED;
> - else
> - mid->mid_state = MID_RESPONSE_MALFORMED;
> - /*
> - * Trying to handle/dequeue a mid after the send_recv()
> - * function has finished processing it is a bug.
> - */
> if (mid->mid_flags & MID_DELETED) {
> - spin_unlock(&GlobalMid_Lock);
> pr_warn_once("trying to dequeue a deleted mid\n");
> - } else {
> - list_del_init(&mid->qhead);
> - mid->mid_flags |= MID_DELETED;
> - spin_unlock(&GlobalMid_Lock);
> + return;
> }
> +
> + spin_lock(&GlobalMid_Lock);
> + list_del_init(&mid->qhead);
> + mid->mid_flags |= MID_DELETED;
> + spin_unlock(&GlobalMid_Lock);
> }
>
> static unsigned int
> @@ -883,15 +875,16 @@ smb2_get_credits_from_hdr(char *buffer, struct TCP_Server_Info *server)
> }
>
> static void
> -handle_mid(struct mid_q_entry *mid, struct TCP_Server_Info *server,
> - char *buf, int malformed)
> +handle_mid(struct mid_q_entry *mid, struct TCP_Server_Info *server, char *buf)
> {
> if (server->ops->check_trans2 &&
> - server->ops->check_trans2(mid, server, buf, malformed))
> + server->ops->check_trans2(mid, server, buf))
> return;
> +
> mid->credits_received = smb2_get_credits_from_hdr(buf, server);
> mid->resp_buf = buf;
> mid->large_buf = server->large_buf;
> +
> /* Was previous buf put in mpx struct for multi-rsp? */
> if (!mid->multiRsp) {
> /* smb buffer will be freed by user thread */
> @@ -900,7 +893,8 @@ handle_mid(struct mid_q_entry *mid, struct TCP_Server_Info *server,
> else
> server->smallbuf = NULL;
> }
> - dequeue_mid(mid, malformed);
> +
> + dequeue_mid(mid);
> }
>
> static void clean_demultiplex_info(struct TCP_Server_Info *server)
> @@ -1050,9 +1044,6 @@ cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> * into the payload for debugging purposes.
> */
> rc = server->ops->check_message(buf, server->total_read, server);
> - if (rc)
> - cifs_dump_mem("Bad SMB: ", buf,
> - min_t(unsigned int, server->total_read, 48));
>
> if (server->ops->is_session_expired &&
> server->ops->is_session_expired(buf)) {
> @@ -1067,7 +1058,16 @@ cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> if (!mid)
> return rc;
>
> - handle_mid(mid, server, buf, rc);
> + if (unlikely(rc)) {
> + cifs_dump_mem("Bad SMB: ", buf,
> + min_t(unsigned int, server->total_read, 48));
> + /* mid is malformed */
> + mid->mid_state = MID_RESPONSE_MALFORMED;
> + } else {
> + mid->mid_state = MID_RESPONSE_RECEIVED;
> + }
> +
> + handle_mid(mid, server, buf);
> return 0;
> }
>
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index 2e20ee4dab7b..416293fe14fb 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -384,21 +384,21 @@ cifs_downgrade_oplock(struct TCP_Server_Info *server,
>
> static bool
> cifs_check_trans2(struct mid_q_entry *mid, struct TCP_Server_Info *server,
> - char *buf, int malformed)
> + char *buf)
> {
> - if (malformed)
> - return false;
> if (check2ndT2(buf) <= 0)
> return false;
> mid->multiRsp = true;
> if (mid->resp_buf) {
> + int rc;
> /* merge response - fix up 1st*/
> - malformed = coalesce_t2(buf, mid->resp_buf);
> - if (malformed > 0)
> + rc = coalesce_t2(buf, mid->resp_buf);
> + if (rc > 0)
> return true;
> /* All parts received or packet is malformed. */
> mid->multiEnd = true;
> - dequeue_mid(mid, malformed);
> + mid->mid_state = MID_RESPONSE_RECEIVED;
> + dequeue_mid(mid);
> return true;
> }
> if (!server->large_buf) {
> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> index 562064fe9668..0d57341e4f4a 100644
> --- a/fs/cifs/smb2misc.c
> +++ b/fs/cifs/smb2misc.c
> @@ -26,17 +26,15 @@ check_smb2_hdr(struct smb2_hdr *shdr, __u64 mid)
> * Make sure that this really is an SMB, that it is a response,
> * and that the message ids match.
> */
> - if ((shdr->ProtocolId == SMB2_PROTO_NUMBER) &&
> - (mid == wire_mid)) {
> + if ((shdr->ProtocolId == SMB2_PROTO_NUMBER) && (mid == wire_mid)) {
> if (shdr->Flags & SMB2_FLAGS_SERVER_TO_REDIR)
> return 0;
> - else {
> - /* only one valid case where server sends us request */
> - if (shdr->Command == SMB2_OPLOCK_BREAK)
> - return 0;
> - else
> - cifs_dbg(VFS, "Received Request not response\n");
> - }
> +
> + /* only one valid case where server sends us request */
> + if (shdr->Command == SMB2_OPLOCK_BREAK)
> + return 0;
> +
> + cifs_dbg(VFS, "Received Request not response\n");
> } else { /* bad signature or mid */
> if (shdr->ProtocolId != SMB2_PROTO_NUMBER)
> cifs_dbg(VFS, "Bad protocol string signature header %x\n",
> @@ -45,7 +43,7 @@ check_smb2_hdr(struct smb2_hdr *shdr, __u64 mid)
> cifs_dbg(VFS, "Mids do not match: %llu and %llu\n",
> mid, wire_mid);
> }
> - cifs_dbg(VFS, "Bad SMB detected. The Mid=%llu\n", wire_mid);
> + cifs_dbg(VFS, "Bad SMB detected, mid=%llu\n", wire_mid);
> return 1;
> }
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 8802995b2d3d..f63139015afa 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -335,7 +335,7 @@ smb2_revert_current_mid(struct TCP_Server_Info *server, const unsigned int val)
> }
>
> static struct mid_q_entry *
> -__smb2_find_mid(struct TCP_Server_Info *server, char *buf, bool dequeue)
> +smb2_find_mid(struct TCP_Server_Info *server, char *buf)
> {
> struct mid_q_entry *mid;
> struct smb2_hdr *shdr = (struct smb2_hdr *)buf;
> @@ -352,10 +352,6 @@ __smb2_find_mid(struct TCP_Server_Info *server, char *buf, bool dequeue)
> (mid->mid_state == MID_REQUEST_SUBMITTED) &&
> (mid->command == shdr->Command)) {
> kref_get(&mid->refcount);
> - if (dequeue) {
> - list_del_init(&mid->qhead);
> - mid->mid_flags |= MID_DELETED;
> - }
> spin_unlock(&GlobalMid_Lock);
> return mid;
> }
> @@ -364,18 +360,6 @@ __smb2_find_mid(struct TCP_Server_Info *server, char *buf, bool dequeue)
> return NULL;
> }
>
> -static struct mid_q_entry *
> -smb2_find_mid(struct TCP_Server_Info *server, char *buf)
> -{
> - return __smb2_find_mid(server, buf, false);
> -}
> -
> -static struct mid_q_entry *
> -smb2_find_dequeue_mid(struct TCP_Server_Info *server, char *buf)
> -{
> - return __smb2_find_mid(server, buf, true);
> -}
> -
> static void
> smb2_dump_detail(void *buf, struct TCP_Server_Info *server)
> {
> @@ -4912,7 +4896,7 @@ handle_read_data(struct TCP_Server_Info *server, struct mid_q_entry *mid,
> }
>
> if (server->ops->is_status_pending &&
> - server->ops->is_status_pending(buf, server))
> + server->ops->is_status_pending(buf, server))
> return -1;
>
> /* set up first two iov to get credits */
> @@ -4931,11 +4915,9 @@ handle_read_data(struct TCP_Server_Info *server, struct mid_q_entry *mid,
> cifs_dbg(FYI, "%s: server returned error %d\n",
> __func__, rdata->result);
> /* normal error on read response */
> - if (is_offloaded)
> - mid->mid_state = MID_RESPONSE_RECEIVED;
> - else
> - dequeue_mid(mid, false);
> - return 0;
> + mid->mid_state = MID_RESPONSE_RECEIVED;
> + length = 0;
> + goto err_out;
> }
>
> data_offset = server->ops->read_data_offset(buf);
> @@ -4958,11 +4940,7 @@ handle_read_data(struct TCP_Server_Info *server, struct mid_q_entry *mid,
> cifs_dbg(FYI, "%s: data offset (%u) beyond end of smallbuf\n",
> __func__, data_offset);
> rdata->result = -EIO;
> - if (is_offloaded)
> - mid->mid_state = MID_RESPONSE_MALFORMED;
> - else
> - dequeue_mid(mid, rdata->result);
> - return 0;
> + goto err_malformed;
> }
>
> pad_len = data_offset - server->vals->read_rsp_size;
> @@ -4977,32 +4955,19 @@ handle_read_data(struct TCP_Server_Info *server, struct mid_q_entry *mid,
> cifs_dbg(FYI, "%s: data offset (%u) beyond 1st page of response\n",
> __func__, data_offset);
> rdata->result = -EIO;
> - if (is_offloaded)
> - mid->mid_state = MID_RESPONSE_MALFORMED;
> - else
> - dequeue_mid(mid, rdata->result);
> - return 0;
> + goto err_malformed;
> }
>
> if (data_len > page_data_size - pad_len) {
> /* data_len is corrupt -- discard frame */
> rdata->result = -EIO;
> - if (is_offloaded)
> - mid->mid_state = MID_RESPONSE_MALFORMED;
> - else
> - dequeue_mid(mid, rdata->result);
> - return 0;
> + goto err_malformed;
> }
>
> rdata->result = init_read_bvec(pages, npages, page_data_size,
> cur_off, &bvec);
> - if (rdata->result != 0) {
> - if (is_offloaded)
> - mid->mid_state = MID_RESPONSE_MALFORMED;
> - else
> - dequeue_mid(mid, rdata->result);
> - return 0;
> - }
> + if (rdata->result != 0)
> + goto err_malformed;
>
> iov_iter_bvec(&iter, WRITE, bvec, npages, data_len);
> } else if (buf_len >= data_offset + data_len) {
> @@ -5015,24 +4980,26 @@ handle_read_data(struct TCP_Server_Info *server, struct mid_q_entry *mid,
> /* read response payload cannot be in both buf and pages */
> WARN_ONCE(1, "buf can not contain only a part of read data");
> rdata->result = -EIO;
> - if (is_offloaded)
> - mid->mid_state = MID_RESPONSE_MALFORMED;
> - else
> - dequeue_mid(mid, rdata->result);
> - return 0;
> + goto err_malformed;
> }
>
> length = rdata->copy_into_pages(server, rdata, &iter);
>
> kfree(bvec);
>
> - if (length < 0)
> +err_out:
> + if (length <= 0)
> return length;
>
> - if (is_offloaded)
> - mid->mid_state = MID_RESPONSE_RECEIVED;
> +err_malformed:
> + if (rdata->result != 0)
> + mid->mid_state = MID_RESPONSE_MALFORMED;
> else
> - dequeue_mid(mid, false);
> + mid->mid_state = MID_RESPONSE_RECEIVED;
> +
> + if (!is_offloaded)
> + dequeue_mid(mid);
> +
> return length;
> }
>
> @@ -5061,44 +5028,45 @@ static void smb2_decrypt_offload(struct work_struct *work)
> }
>
> dw->server->lstrp = jiffies;
> - mid = smb2_find_dequeue_mid(dw->server, dw->buf);
> - if (mid == NULL)
> + mid = smb2_find_mid(dw->server, dw->buf);
> + if (mid == NULL) {
> cifs_dbg(FYI, "mid not found\n");
> - else {
> - mid->decrypted = true;
> - rc = handle_read_data(dw->server, mid, dw->buf,
> - dw->server->vals->read_rsp_size,
> - dw->ppages, dw->npages, dw->len,
> - true);
> - if (rc >= 0) {
> + goto free_pages;
> + }
> +
> + dequeue_mid(mid);
> +
> + mid->decrypted = true;
> + rc = handle_read_data(dw->server, mid, dw->buf,
> + dw->server->vals->read_rsp_size, dw->ppages,
> + dw->npages, dw->len, true);
> + if (rc >= 0) {
> #ifdef CONFIG_CIFS_STATS2
> - mid->when_received = jiffies;
> + mid->when_received = jiffies;
> #endif
> - if (dw->server->ops->is_network_name_deleted)
> - dw->server->ops->is_network_name_deleted(dw->buf,
> - dw->server);
> + if (dw->server->ops->is_network_name_deleted)
> + dw->server->ops->is_network_name_deleted(dw->buf, dw->server);
>
> - mid->callback(mid);
> + mid->callback(mid);
> + } else {
> + spin_lock(&cifs_tcp_ses_lock);
> + spin_lock(&GlobalMid_Lock);
> + if (dw->server->tcpStatus == CifsNeedReconnect) {
> + mid->mid_state = MID_RETRY_NEEDED;
> } else {
> - spin_lock(&cifs_tcp_ses_lock);
> - spin_lock(&GlobalMid_Lock);
> - if (dw->server->tcpStatus == CifsNeedReconnect) {
> - mid->mid_state = MID_RETRY_NEEDED;
> - spin_unlock(&GlobalMid_Lock);
> - spin_unlock(&cifs_tcp_ses_lock);
> - mid->callback(mid);
> - } else {
> - mid->mid_state = MID_REQUEST_SUBMITTED;
> - mid->mid_flags &= ~(MID_DELETED);
> - list_add_tail(&mid->qhead,
> - &dw->server->pending_mid_q);
> - spin_unlock(&GlobalMid_Lock);
> - spin_unlock(&cifs_tcp_ses_lock);
> - }
> + mid->mid_state = MID_REQUEST_SUBMITTED;
> + mid->mid_flags &= ~(MID_DELETED);
> + list_add_tail(&mid->qhead, &dw->server->pending_mid_q);
> }
> - cifs_mid_q_entry_release(mid);
> + spin_unlock(&GlobalMid_Lock);
> + spin_unlock(&cifs_tcp_ses_lock);
> +
> + if (mid->mid_state == MID_RETRY_NEEDED)
> + mid->callback(mid);
> }
>
> + cifs_mid_q_entry_release(mid);
> +
> free_pages:
> for (i = dw->npages-1; i >= 0; i--)
> put_page(dw->ppages[i]);
> @@ -5191,22 +5159,18 @@ receive_encrypted_read(struct TCP_Server_Info *server, struct mid_q_entry **mid,
> goto free_pages;
>
> *mid = smb2_find_mid(server, buf);
> - if (*mid == NULL)
> + if (*mid == NULL) {
> cifs_dbg(FYI, "mid not found\n");
> - else {
> - cifs_dbg(FYI, "mid found\n");
> - (*mid)->decrypted = true;
> - rc = handle_read_data(server, *mid, buf,
> - server->vals->read_rsp_size,
> - pages, npages, len, false);
> - if (rc >= 0) {
> - if (server->ops->is_network_name_deleted) {
> - server->ops->is_network_name_deleted(buf,
> - server);
> - }
> - }
> + goto free_pages;
> }
>
> + (*mid)->decrypted = true;
> + rc = handle_read_data(server, *mid, buf, server->vals->read_rsp_size,
> + pages, npages, len, false);
> + if (rc >= 0)
> + if (server->ops->is_network_name_deleted)
> + server->ops->is_network_name_deleted(buf, server);
> +
> free_pages:
> for (i = i - 1; i >= 0; i--)
> put_page(pages[i]);
> @@ -5253,7 +5217,7 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
> return length;
>
> next_is_large = server->large_buf;
> -one_more:
> +next:
> shdr = (struct smb2_hdr *)buf;
> if (shdr->NextCommand) {
> if (next_is_large)
> @@ -5266,10 +5230,7 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
> }
>
> mid_entry = smb2_find_mid(server, buf);
> - if (mid_entry == NULL)
> - cifs_dbg(FYI, "mid not found\n");
> - else {
> - cifs_dbg(FYI, "mid found\n");
> + if (mid_entry) {
> mid_entry->decrypted = true;
> mid_entry->resp_buf_size = server->pdu_size;
> }
> @@ -5278,6 +5239,7 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
> cifs_server_dbg(VFS, "too many PDUs in compound\n");
> return -1;
> }
> +
> bufs[*num_mids] = buf;
> mids[(*num_mids)++] = mid_entry;
>
> @@ -5293,7 +5255,7 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
> server->bigbuf = buf = next_buffer;
> else
> server->smallbuf = buf = next_buffer;
> - goto one_more;
> + goto next;
> } else if (ret != 0) {
> /*
> * ret != 0 here means that we didn't get to handle_mid() thus
> --
> 2.35.3
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] smb2: small refactor in smb2_check_message()
2022-07-21 3:05 ` Steve French
@ 2022-07-21 20:47 ` Enzo Matsumiya
0 siblings, 0 replies; 6+ messages in thread
From: Enzo Matsumiya @ 2022-07-21 20:47 UTC (permalink / raw)
To: Steve French; +Cc: CIFS, Paulo Alcantara, ronnie sahlberg, Shyam Prasad N
On 07/20, Steve French wrote:
>I don't have any objections to this but wondering what prompted the
>patch? Did you see an error logged with ioctls? You mention:
>
>"SMB2_IOCTL, OutputLength and OutputContext are optional and can be zero"
No errors, it's just that when the IOCTL have a 0 len, there can be
several messages printed when debugging. It wasn't that much, but enough
to get me confused several times.
Since they were completely unnecessary, I thought it was ok to not print it.
>And did you want to change the pr_warn_once to a pr_warn on the
>mismatch since you had a case where server was frequently returning
>frame with bad length and you want to debug it?
Yes. I'm always getting a lot of those mismatched length calculations
(Windows Server 2019). After investigating, I found that they were not
real mismatches (see below).
>I am a little worried that it could cause log spam if some server has
>a bug in smb3 response length.
Yes, it could, but as I mention in the commit message, this would only
happen on the *real* mismatched cases, i.e. after passing all the checks
inside the "if (len != calc_len)" check. In those cases, I'd prefer to
really be aware of them then risk it getting lost in dmesg.
Cheers,
Enzo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] smb2: simplify mid handling/dequeueing code
2022-07-21 3:26 ` [PATCH] smb2: simplify mid handling/dequeueing code Steve French
@ 2022-07-21 21:02 ` Enzo Matsumiya
0 siblings, 0 replies; 6+ messages in thread
From: Enzo Matsumiya @ 2022-07-21 21:02 UTC (permalink / raw)
To: Steve French; +Cc: rohiths msft, CIFS
On 07/20, Steve French wrote:
>Can you give some context as to why only smb2_find_dequeue_mid()
>set the "dequeue" flag to true and why it doesn't need to be
>distinguished from the other find_mid cases?
>I think Rohith wrote this - Rohith does the change make sense to you? Thoughts?
The way I see it, it's better to be explicit and do a find_mid()
followed by a dequeue_mid(), than to have a function that do both,
especially when that function is used only in one place
(smb2_decrypt_offload()).
Tho @Rohith please clarify if I missed something here.
>Also Enzo can you explain a little what the callers were who set
>"malformed" (and why it now no longer needs to be used)?
This is, again, to be explicit. It makes no sense to me to have a
"malformed" parameter in a function named "dequeue_mid", e.g. when
reading the code I stumbled upon:
dequeue_mid(mid, false);
and
dequeue_mid(mid, rdata->result);
Let me know if this doesn't make sense to you.
Cheers,
Enzo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-21 21:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-19 17:31 [PATCH] smb2: simplify mid handling/dequeueing code Enzo Matsumiya
2022-07-19 17:31 ` [PATCH] smb2: small refactor in smb2_check_message() Enzo Matsumiya
2022-07-21 3:05 ` Steve French
2022-07-21 20:47 ` Enzo Matsumiya
2022-07-21 3:26 ` [PATCH] smb2: simplify mid handling/dequeueing code Steve French
2022-07-21 21:02 ` Enzo Matsumiya
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.