Handle mismatched open calls

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

Quoted 35 lines · Dan Carpenter

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-

Quoted 1 line

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;

Quoted 2 lines

regards,
dan carpenter

Thanks Dan,

Quoted 36 lines · Sachin Prabhu

Quoted 35 lines · Dan Carpenter

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-

Quoted 1 line

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;

Quoted 5 lines

Quoted 2 lines

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

fs/cifs/connect.c +8 -2

+				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,

fs/cifs/smb2misc.c +1 -0

index fd516ea..f50e3ef 100644
@@ -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;
 }
+

fs/cifs/smb2misc.c +4 -0

+	cifs_put_tcon(cancelled->tcon);
+	kfree(cancelled);
+}
+

fs/cifs/smb2misc.c +18 -0

+	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;
+}

fs/cifs/smb2transport.c +8 -2

 {
 	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;

fs/cifs/smb2transport.c +29 -0

+		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)
 {

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...

Quoted 13 lines

fs/cifs/connect.c +0 -0

index 9ae695a..131663b 100644
@@ -904,10 +904,19 @@ cifs_demultiplex_thread(void *p)

  server->lstrp = jiffies;
  if (mid_entry != NULL) {

fs/cifs/connect.c +1 -0

+			if ((mid_entry->mid_flags &

MID_WAIT_CANCELLED) &&

fs/cifs/connect.c +1 -0

+			     mid_entry->mid_state ==

MID_RESPONSE_RECEIVED &&

fs/cifs/connect.c +1 -0

+					(server->ops-

Quoted 1 line

Quoted 1 line

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)

Quoted 44 lines

fs/cifs/connect.c +2 -0

+				server->ops->handle_cancelled_mid(
+							mid_entry-

Quoted 1 line

resp_buf,

fs/cifs/connect.c +2 -0

+							server);
+

  if (!mid_entry->multiRsp || mid_entry-

Quoted 1 line

multiEnd)

  mid_entry->callback(mid_entry);

fs/cifs/connect.c +0 -2

-		} else if (!server->ops->is_oplock_break ||
-			   !server->ops->is_oplock_break(buf,

server)) {

fs/cifs/connect.c +2 -0

+		} else if (server->ops->is_oplock_break &&
+			   server->ops->is_oplock_break(buf,

server)) {

fs/cifs/connect.c +2 -0

+			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,

fs/cifs/smb2misc.c +0 -0

index fd516ea..f50e3ef 100644
@@ -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;
 }

fs/cifs/smb2misc.c +5 -0

+
+void
+smb2_cancelled_close_fid(struct work_struct *work)
+{
+	struct close_cancelled_open *cancelled =

container_of(work,

fs/cifs/smb2misc.c +1 -0

+					struct

close_cancelled_open, work);

fs/cifs/smb2misc.c +4 -0

+
+	cifs_dbg(VFS, "Close unmatched open\n");
+
+	SMB2_close(0, cancelled->tcon, cancelled-

Quoted 1 line

fid.persistent_fid,

fs/cifs/smb2misc.c +1 -0

+					   cancelled-

Quoted 1 line

Quoted 1 line

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);

Quoted 11 lines

fs/cifs/smb2misc.c +6 -0

+	cifs_put_tcon(cancelled->tcon);
+	kfree(cancelled);
+}
+
+int
+smb2_handle_cancelled_mid(char *buffer, struct TCP_Server_Info

*server)

fs/cifs/smb2misc.c +3 -0

+{
+	struct smb2_sync_hdr *sync_hdr = get_sync_hdr(buffer);
+	struct smb2_create_rsp *rsp = (struct smb2_create_rsp

*)buffer;

fs/cifs/smb2misc.c

Quoted 5 lines

fs/cifs/smb2misc.c +5 -0

+	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;

fs/cifs/smb2misc.c

Quoted 6 lines

fs/cifs/smb2misc.c +4 -0

+		return 0;
+
+	cancelled = (struct close_cancelled_open *)
+			kzalloc(sizeof(struct

close_cancelled_open),

fs/cifs/smb2misc.c +1 -0

+				GFP_KERNEL);

No need to cast kzalloc.  Use sizeof(*cancelled).

	cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL);

Quoted 18 lines

fs/cifs/smb2misc.c +18 -0

+	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 ]

fs/cifs/smb2transport.c

Quoted 13 lines

fs/cifs/smb2transport.c +0 -0

index 7c3bb1b..e3c8a9c 100644
@@ -115,23 +115,72 @@ smb3_crypto_shash_allocate(struct
 TCP_Server_Info *server)
  	return 0;
  }

fs/cifs/smb2transport.c +2 -2

-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?

Quoted 24 lines

 {
  struct cifs_ses *ses;

fs/cifs/smb2transport.c +0 -1

-	spin_lock(&cifs_tcp_ses_lock);

  list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
  if (ses->Suid != ses_id) continue;

fs/cifs/smb2transport.c +0 -1

-		spin_unlock(&cifs_tcp_ses_lock);

  return ses;
  }

fs/cifs/smb2transport.c +11 -0

+
+	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);

fs/cifs/smb2transport.c

Quoted 5 lines

fs/cifs/smb2transport.c +5 -0

+	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.

Quoted 7 lines

fs/cifs/smb2transport.c +6 -0

+{
+	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()?

Quoted 42 lines

fs/cifs/smb2transport.c +6 -0

+		if (tcon->tid != tid)
+			continue;
+		++tcon->tc_count;
+		return tcon;
+	}
+

  return NULL;
 }

fs/cifs/smb2transport.c +7 -0

+/*
+ * 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)

fs/cifs/smb2transport.c +16 -0

+{
+	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)
 {

fs/cifs/smb2transport.c +0 -0

@@ -511,6 +560,7 @@ smb2_mid_entry_alloc(const struct smb2_sync_hdr
 *shdr,

  atomic_inc(&midCount);
  temp->mid_state = MID_REQUEST_ALLOCATED;

fs/cifs/smb2transport.c

Quoted 1 line

fs/cifs/smb2transport.c +1 -0

+	temp->mid_flags = 0;

No need.  We already memset() this to zero.

Quoted 12 lines

  return temp;
 }

fs/cifs/transport.c +0 -0

index 526f053..d82a4ae 100644
@@ -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) {

fs/cifs/transport.c

Quoted 2 lines

fs/cifs/transport.c +2 -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-

Quoted 1 line

mid);

Quoted 7 lines

  send_cancel(ses->server, rqst, midQ); spin_lock(&GlobalMid_Lock);
  if (midQ->mid_state == MID_REQUEST_SUBMITTED) {

fs/cifs/transport.c +1 -0

+			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 [email protected] More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Dan Carpenter

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

Quoted 283 lines · Sachin Prabhu

Quoted 9 lines

fs/cifs/connect.c +1 -0

index 9ae695a..131663b 100644
@@ -904,10 +904,19 @@ cifs_demultiplex_thread(void *p)
 
            server->lstrp = jiffies;
            if (mid_entry != NULL) {
+                   if ((mid_entry->mid_flags &

Quoted 5 lines

MID_WAIT_CANCELLED) &&
+ mid_entry->mid_state ==
MID_RESPONSE_RECEIVED &&
+ (server->ops-

Quoted 1 line

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)

Quoted 43 lines

fs/cifs/connect.c +2 -0

+                           server->ops->handle_cancelled_mid(
+                                                   mid_entry-

Quoted 1 line

resp_buf,

fs/cifs/connect.c +2 -0

+                                                   server);
+
                    if (!mid_entry->multiRsp || mid_entry-

Quoted 1 line

multiEnd)

                            mid_entry->callback(mid_entry);

fs/cifs/connect.c +0 -2

-           } else if (!server->ops->is_oplock_break ||
-                      !server->ops->is_oplock_break(buf,

server)) {

fs/cifs/connect.c +2 -0

+           } else if (server->ops->is_oplock_break &&
+                      server->ops->is_oplock_break(buf,

server)) {

fs/cifs/connect.c +2 -0

+                   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,

fs/cifs/smb2misc.c +5 -0

index fd516ea..f50e3ef 100644
@@ -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);

fs/cifs/smb2misc.c +4 -0

+
+   cifs_dbg(VFS, "Close unmatched open\n");
+
+   SMB2_close(0, cancelled->tcon, cancelled-

Quoted 1 line

fid.persistent_fid,

Quoted 2 lines

+ cancelled-

Quoted 1 line

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);

Quoted 11 lines

fs/cifs/smb2misc.c +6 -0

+   cifs_put_tcon(cancelled->tcon);
+   kfree(cancelled);
+}
+
+int
+smb2_handle_cancelled_mid(char *buffer, struct TCP_Server_Info

*server)

fs/cifs/smb2misc.c +3 -0

+{
+   struct smb2_sync_hdr *sync_hdr = get_sync_hdr(buffer);
+   struct smb2_create_rsp *rsp = (struct smb2_create_rsp

*)buffer;

fs/cifs/smb2misc.c smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server)

Quoted 5 lines

fs/cifs/smb2misc.c +5 -0

+   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;

Quoted 6 lines

fs/cifs/smb2misc.c +4 -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);

Quoted 18 lines

fs/cifs/smb2misc.c +18 -0

+   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 ]

fs/cifs/smb2transport.c smb3_crypto_shash_allocate(struct · line 120

Quoted 13 lines

fs/cifs/smb2transport.c +2 -2

index 7c3bb1b..e3c8a9c 100644
@@ -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?

Quoted 19 lines

fs/cifs/smb2transport.c +8 -2

 {
    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;

fs/cifs/smb2transport.c smb3_crypto_shash_allocate(struct TCP_Server_Info *server)

Quoted 10 lines

fs/cifs/smb2transport.c +8 -0

+
+   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.

fs/cifs/smb2transport.c smb3_crypto_shash_allocate(struct · line 154

Quoted 7 lines

fs/cifs/smb2transport.c +6 -0

+{
+   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()?

Quoted 38 lines

fs/cifs/smb2transport.c +13 -0

+           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)

fs/cifs/smb2transport.c +16 -0

+{
+   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)
{

fs/cifs/smb2transport.c smb2_mid_entry_alloc(const struct smb2_sync_hdr · line 564

Quoted 5 lines

fs/cifs/smb2transport.c +1 -0

@@ -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.

Quoted 8 lines

fs/cifs/smb2transport.c +0 -0

    return temp;
 }

fs/cifs/transport.c +0 -0

index 526f053..d82a4ae 100644

fs/cifs/transport.c cifs_send_recv(const unsigned int xid, struct · line 757

Quoted 6 lines

fs/cifs/transport.c +2 -0

@@ -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-

fs/cifs/transport.c cifs_send_recv(const unsigned int xid, struct cifs_ses *ses, · line 762

Quoted 10 lines

fs/cifs/transport.c +1 -0

 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

merged into cifs-2.6.git for-next

Quoted 11 lines · Sachin Prabhu

Quoted 4 lines · Dan Carpenter

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.

Quoted 292 lines

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

Quoted 283 lines · Sachin Prabhu

Quoted 9 lines

fs/cifs/connect.c +1 -0

index 9ae695a..131663b 100644
@@ -904,10 +904,19 @@ cifs_demultiplex_thread(void *p)
 
            server->lstrp = jiffies;
            if (mid_entry != NULL) {
+                   if ((mid_entry->mid_flags &

Quoted 5 lines

MID_WAIT_CANCELLED) &&
+ mid_entry->mid_state ==
MID_RESPONSE_RECEIVED &&
+ (server->ops-

Quoted 1 line

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)

Quoted 43 lines

fs/cifs/connect.c +2 -0

+                           server->ops->handle_cancelled_mid(
+                                                   mid_entry-

Quoted 1 line

resp_buf,

fs/cifs/connect.c +2 -0

+                                                   server);
+
                    if (!mid_entry->multiRsp || mid_entry-

Quoted 1 line

multiEnd)

                            mid_entry->callback(mid_entry);

fs/cifs/connect.c +0 -2

-           } else if (!server->ops->is_oplock_break ||
-                      !server->ops->is_oplock_break(buf,

server)) {

fs/cifs/connect.c +2 -0

+           } else if (server->ops->is_oplock_break &&
+                      server->ops->is_oplock_break(buf,

server)) {

fs/cifs/connect.c +2 -0

+                   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,

fs/cifs/smb2misc.c +5 -0

index fd516ea..f50e3ef 100644
@@ -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);

fs/cifs/smb2misc.c +4 -0

+
+   cifs_dbg(VFS, "Close unmatched open\n");
+
+   SMB2_close(0, cancelled->tcon, cancelled-

Quoted 1 line

fid.persistent_fid,

Quoted 2 lines

+ cancelled-

Quoted 1 line

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);

Quoted 11 lines

fs/cifs/smb2misc.c +6 -0

+   cifs_put_tcon(cancelled->tcon);
+   kfree(cancelled);
+}
+
+int
+smb2_handle_cancelled_mid(char *buffer, struct TCP_Server_Info

*server)

fs/cifs/smb2misc.c +3 -0

+{
+   struct smb2_sync_hdr *sync_hdr = get_sync_hdr(buffer);
+   struct smb2_create_rsp *rsp = (struct smb2_create_rsp

*)buffer;

fs/cifs/smb2misc.c smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server)

Quoted 5 lines

fs/cifs/smb2misc.c +5 -0

+   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;

Quoted 6 lines

fs/cifs/smb2misc.c +4 -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);

Quoted 18 lines

fs/cifs/smb2misc.c +18 -0

+   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 ]

fs/cifs/smb2transport.c smb3_crypto_shash_allocate(struct · line 120

Quoted 13 lines

fs/cifs/smb2transport.c +2 -2

index 7c3bb1b..e3c8a9c 100644
@@ -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?

Quoted 19 lines

fs/cifs/smb2transport.c +8 -2

 {
    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;

fs/cifs/smb2transport.c smb3_crypto_shash_allocate(struct TCP_Server_Info *server)

Quoted 10 lines

fs/cifs/smb2transport.c +8 -0

+
+   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.

fs/cifs/smb2transport.c smb3_crypto_shash_allocate(struct · line 154

Quoted 7 lines

fs/cifs/smb2transport.c +6 -0

+{
+   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()?

Quoted 38 lines

fs/cifs/smb2transport.c +13 -0

+           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)

fs/cifs/smb2transport.c +16 -0

+{
+   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)
{

fs/cifs/smb2transport.c smb2_mid_entry_alloc(const struct smb2_sync_hdr · line 564

Quoted 5 lines

fs/cifs/smb2transport.c +1 -0

@@ -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.

Quoted 8 lines

fs/cifs/smb2transport.c +0 -0

    return temp;
 }

fs/cifs/transport.c +0 -0

index 526f053..d82a4ae 100644

fs/cifs/transport.c cifs_send_recv(const unsigned int xid, struct · line 757

Quoted 6 lines

fs/cifs/transport.c +2 -0

@@ -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-

fs/cifs/transport.c cifs_send_recv(const unsigned int xid, struct cifs_ses *ses, · line 762

Quoted 10 lines

fs/cifs/transport.c +1 -0

 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Thanks,

Steve