CIFS: Move readpage code to ops struct

DISCUSSION Dan Carpenter 3 participants 3 messages Oct 25, 2012 1 message not in the archive

Subsystems (2)

SMB client (CIFS) Kernel SMB3 server (ksmbd)

lore

Share & tools

lore

Dan Carpenter

author

2012-10-25 11:28 UTC #0 lore raw

Hello Pavel Shilovsky,

This is a semi-automatic email about new static checker warnings.

The patch f9c6e234c3ca: "CIFS: Move readpage code to ops struct" from Sep 18, 2012, leads to the following Smatch complaint:

fs/cifs/file.c:2954 cifs_read()

	 warn: variable dereferenced before check 'tcon->ses' (see line 2932)

fs/cifs/file.c
2931 tcon = tlink_tcon(open_file->tlink);

  2932		server = tcon->ses->server;
                         ^^^^^^^^^^^

New dereference.

2933
2934 if (!server->ops->sync_read) {
2935 free_xid(xid);
2936 return -ENOSYS;
2937 }
2938
2939 if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_RWPIDFORWARD)
2940 pid = open_file->pid;
2941 else
2942 pid = current->tgid;
2943
2944 if ((file->f_flags & O_ACCMODE) == O_WRONLY)
2945 cFYI(1, "attempting read on write only file instance");
2946
2947 for (total_read = 0, cur_offset = read_data; read_size > total_read;
2948 total_read += bytes_read, cur_offset += bytes_read) {
2949 current_read_size = min_t(uint, read_size - total_read, rsize);
2950 /*
2951 * For windows me and 9x we do not want to request more than it
2952 * negotiated since it will refuse the read then.
2953 */
2954 if ((tcon->ses) && !(tcon->ses->capabilities &

                             ^^^^^^^^^

Old check.

2955 tcon->ses->server->vals->cap_large_files)) {
2956 current_read_size = min_t(uint, current_read_size,

regards,
dan carpenter

A message that is not in the archive (sent off-list or not yet received): [email protected]

Pavel Shilovsky

2012-10-25 13:34 UTC #2 lore raw

2012/10/25 Dan Carpenter <[email protected]>:

Quoted 34 lines

Hello Pavel Shilovsky,
This is a semi-automatic email about new static checker warnings.
The patch f9c6e234c3ca: "CIFS: Move readpage code to ops struct" from Sep 18, 2012, leads to the following Smatch complaint:
fs/cifs/file.c:2954 cifs_read()
         warn: variable dereferenced before check 'tcon->ses' (see line 2932)
fs/cifs/file.c
2931 tcon = tlink_tcon(open_file->tlink);
  2932          server = tcon->ses->server;
                         ^^^^^^^^^^^
New dereference.
2933
2934 if (!server->ops->sync_read) {
2935 free_xid(xid);
2936 return -ENOSYS;
2937 }
2938
2939 if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_RWPIDFORWARD)
2940 pid = open_file->pid;
2941 else
2942 pid = current->tgid;
2943
2944 if ((file->f_flags & O_ACCMODE) == O_WRONLY)
2945 cFYI(1, "attempting read on write only file instance");
2946
2947 for (total_read = 0, cur_offset = read_data; read_size > total_read;
2948 total_read += bytes_read, cur_offset += bytes_read) {
2949 current_read_size = min_t(uint, read_size - total_read, rsize);
2950 /*

Quoted 8 lines

  2951                   * For windows me and 9x we do not want to request more than it
  2952                   * negotiated since it will refuse the read then.
  2953                   */
  2954                  if ((tcon->ses) && !(tcon->ses->capabilities &
                             ^^^^^^^^^

Old check.

  2955                                  tcon->ses->server->vals->cap_large_files)) {

I don't think that tcon->ses can be NULL here - it seems that we can remove this check. Thoughts?

Quoted 8 lines

2956 current_read_size = min_t(uint, current_read_size,
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

Signature · 2 lines

Best regards,
Pavel Shilovsky.

Jeff Layton

2012-10-25 13:50 UTC #3 lore raw

Quoted 42 lines · On Thu, 25 Oct 2012 17:34:38 +0400 Pavel Shilovsky…

2012/10/25 Dan Carpenter <[email protected]>:
Quoted 41 lines
Hello Pavel Shilovsky,
This is a semi-automatic email about new static checker warnings.
The patch f9c6e234c3ca: "CIFS: Move readpage code to ops struct" from Sep 18, 2012, leads to the following Smatch complaint:
fs/cifs/file.c:2954 cifs_read()
         warn: variable dereferenced before check 'tcon->ses' (see line 2932)
fs/cifs/file.c
2931 tcon = tlink_tcon(open_file->tlink);
  2932          server = tcon->ses->server;
                         ^^^^^^^^^^^
New dereference.
2933
2934 if (!server->ops->sync_read) {
2935 free_xid(xid);
2936 return -ENOSYS;
2937 }
2938
2939 if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_RWPIDFORWARD)
2940 pid = open_file->pid;
2941 else
2942 pid = current->tgid;
2943
2944 if ((file->f_flags & O_ACCMODE) == O_WRONLY)
2945 cFYI(1, "attempting read on write only file instance");
2946
2947 for (total_read = 0, cur_offset = read_data; read_size > total_read;
2948 total_read += bytes_read, cur_offset += bytes_read) {
2949 current_read_size = min_t(uint, read_size - total_read, rsize);
2950 /*
  2951                   * For windows me and 9x we do not want to request more than it
  2952                   * negotiated since it will refuse the read then.
  2953                   */
  2954                  if ((tcon->ses) && !(tcon->ses->capabilities &
                             ^^^^^^^^^
Old check.

Quoted 5 lines

Quoted 1 line
  2955                                  tcon->ses->server->vals->cap_large_files)) {
I don't think that tcon->ses can be NULL here - it seems that we can remove this check. Thoughts?

Agreed. I think that this is a holdover from some really old code before we fixed up all the refcounting and object lifetime. The tcon should be holding a reference to its session object, which in turn holds a reference to the server object. If tcon->ses is NULL then something is very, very wrong. I believe you can just remove that check.

Quoted 11 lines

Quoted 8 lines
2956 current_read_size = min_t(uint, current_read_size,
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

Signature · 1 line

Jeff Layton <[email protected]>