diff mbox

[04/21] cifs: Change SMB2_open to return an iov for the error parameter

Message ID 20180409080646.23621-5-lsahlber@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ronnie Sahlberg April 9, 2018, 8:06 a.m. UTC 
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/smb2ops.c   | 10 ++++++----
 fs/cifs/smb2pdu.c   |  9 +++++----
 fs/cifs/smb2proto.h |  2 +-
 3 files changed, 12 insertions(+), 9 deletions(-)

Comments

Pavel Shilovsky April 12, 2018, 10:08 p.m. UTC | #1 
2018-04-09 1:06 GMT-07:00 Ronnie Sahlberg <lsahlber@redhat.com>:
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/smb2ops.c   | 10 ++++++----
>  fs/cifs/smb2pdu.c   |  9 +++++----
>  fs/cifs/smb2proto.h |  2 +-
>  3 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index def84fed3571..b4ae932ea134 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1451,6 +1451,7 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
>         __u8 oplock = SMB2_OPLOCK_LEVEL_NONE;
>         struct cifs_open_parms oparms;
>         struct cifs_fid fid;
> +       struct kvec err_iov = {NULL, 0};
>         struct smb2_err_rsp *err_buf = NULL;
>         struct smb2_symlink_err_rsp *symlink;
>         unsigned int sub_len;
> @@ -1473,15 +1474,16 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
>         oparms.fid = &fid;
>         oparms.reconnect = false;
>
> -       rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, &err_buf);
> +       rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, &err_iov);
>
>         if (!rc || !err_buf) {
>                 kfree(utf16_path);
>                 return -ENOENT;
>         }
>
> +       err_buf = err_iov.iov_base;
>         if (le32_to_cpu(err_buf->ByteCount) < sizeof(struct smb2_symlink_err_rsp) ||
> -           get_rfc1002_length(err_buf) + server->vals->header_preamble_size < SMB2_SYMLINK_STRUCT_SIZE) {
> +           err_iov.iov_len + server->vals->header_preamble_size < SMB2_SYMLINK_STRUCT_SIZE) {
>                 kfree(utf16_path);
>                 return -ENOENT;
>         }
> @@ -1494,13 +1496,13 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
>         print_len = le16_to_cpu(symlink->PrintNameLength);
>         print_offset = le16_to_cpu(symlink->PrintNameOffset);
>
> -       if (get_rfc1002_length(err_buf) + server->vals->header_preamble_size <
> +       if (err_iov.iov_len + server->vals->header_preamble_size <
>                         SMB2_SYMLINK_STRUCT_SIZE + sub_offset + sub_len) {
>                 kfree(utf16_path);
>                 return -ENOENT;
>         }
>
> -       if (get_rfc1002_length(err_buf) + server->vals->header_preamble_size <
> +       if (err_iov.iov_len + server->vals->header_preamble_size <
>                         SMB2_SYMLINK_STRUCT_SIZE + print_offset + print_len) {
>                 kfree(utf16_path);
>                 return -ENOENT;
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 30dade197e7e..dc66b93131f3 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1701,7 +1701,7 @@ alloc_path_with_tree_prefix(__le16 **out_path, int *out_size, int *out_len,
>  int
>  SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
>           __u8 *oplock, struct smb2_file_all_info *buf,
> -         struct smb2_err_rsp **err_buf)
> +         struct kvec *err_iov)
>  {
>         struct smb2_create_req *req;
>         struct smb2_create_rsp *rsp;
> @@ -1841,9 +1841,10 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
>
>         if (rc != 0) {
>                 cifs_stats_fail_inc(tcon, SMB2_CREATE_HE);
> -               if (err_buf && rsp)
> -                       *err_buf = kmemdup(rsp, get_rfc1002_length(rsp) + 4,
> -                                          GFP_KERNEL);
> +               if (err_iov && rsp) {
> +                       *err_iov = rsp_iov;
> +                       rsp = NULL;

Should we also change a resp_buf_type here to CIFS_NO_BUFFER? This
will prevent cifs_small_buf_release() to complain that a null pointer
is passed.

> +               }
>                 goto creat_exit;
>         }
>
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index cbcce3f7e86f..8ba24a95db71 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -122,7 +122,7 @@ extern int SMB2_tdis(const unsigned int xid, struct cifs_tcon *tcon);
>  extern int SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms,
>                      __le16 *path, __u8 *oplock,
>                      struct smb2_file_all_info *buf,
> -                    struct smb2_err_rsp **err_buf);
> +                    struct kvec *err_iov);
>  extern int SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon,
>                      u64 persistent_fid, u64 volatile_fid, u32 opcode,
>                      bool is_fsctl, char *in_data, u32 indatalen,
> --
> 2.13.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Other than the notice above, the patch looks good.

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

--
Best regards,
Pavel Shilovsky
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ronnie Sahlberg April 12, 2018, 10:13 p.m. UTC | #2 
----- Original Message -----
> From: "Pavel Shilovsky" <piastryyy@gmail.com>
> To: "Ronnie Sahlberg" <lsahlber@redhat.com>
> Cc: "linux-cifs" <linux-cifs@vger.kernel.org>, "Steve French" <smfrench@gmail.com>
> Sent: Friday, 13 April, 2018 8:08:40 AM
> Subject: Re: [PATCH 04/21] cifs: Change SMB2_open to return an iov for the error parameter
> 
> 2018-04-09 1:06 GMT-07:00 Ronnie Sahlberg <lsahlber@redhat.com>:
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/smb2ops.c   | 10 ++++++----
> >  fs/cifs/smb2pdu.c   |  9 +++++----
> >  fs/cifs/smb2proto.h |  2 +-
> >  3 files changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index def84fed3571..b4ae932ea134 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -1451,6 +1451,7 @@ smb2_query_symlink(const unsigned int xid, struct
> > cifs_tcon *tcon,
> >         __u8 oplock = SMB2_OPLOCK_LEVEL_NONE;
> >         struct cifs_open_parms oparms;
> >         struct cifs_fid fid;
> > +       struct kvec err_iov = {NULL, 0};
> >         struct smb2_err_rsp *err_buf = NULL;
> >         struct smb2_symlink_err_rsp *symlink;
> >         unsigned int sub_len;
> > @@ -1473,15 +1474,16 @@ smb2_query_symlink(const unsigned int xid, struct
> > cifs_tcon *tcon,
> >         oparms.fid = &fid;
> >         oparms.reconnect = false;
> >
> > -       rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, &err_buf);
> > +       rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, &err_iov);
> >
> >         if (!rc || !err_buf) {
> >                 kfree(utf16_path);
> >                 return -ENOENT;
> >         }
> >
> > +       err_buf = err_iov.iov_base;
> >         if (le32_to_cpu(err_buf->ByteCount) < sizeof(struct
> >         smb2_symlink_err_rsp) ||
> > -           get_rfc1002_length(err_buf) +
> > server->vals->header_preamble_size < SMB2_SYMLINK_STRUCT_SIZE) {
> > +           err_iov.iov_len + server->vals->header_preamble_size <
> > SMB2_SYMLINK_STRUCT_SIZE) {
> >                 kfree(utf16_path);
> >                 return -ENOENT;
> >         }
> > @@ -1494,13 +1496,13 @@ smb2_query_symlink(const unsigned int xid, struct
> > cifs_tcon *tcon,
> >         print_len = le16_to_cpu(symlink->PrintNameLength);
> >         print_offset = le16_to_cpu(symlink->PrintNameOffset);
> >
> > -       if (get_rfc1002_length(err_buf) +
> > server->vals->header_preamble_size <
> > +       if (err_iov.iov_len + server->vals->header_preamble_size <
> >                         SMB2_SYMLINK_STRUCT_SIZE + sub_offset + sub_len) {
> >                 kfree(utf16_path);
> >                 return -ENOENT;
> >         }
> >
> > -       if (get_rfc1002_length(err_buf) +
> > server->vals->header_preamble_size <
> > +       if (err_iov.iov_len + server->vals->header_preamble_size <
> >                         SMB2_SYMLINK_STRUCT_SIZE + print_offset +
> >                         print_len) {
> >                 kfree(utf16_path);
> >                 return -ENOENT;
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > index 30dade197e7e..dc66b93131f3 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -1701,7 +1701,7 @@ alloc_path_with_tree_prefix(__le16 **out_path, int
> > *out_size, int *out_len,
> >  int
> >  SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16
> >  *path,
> >           __u8 *oplock, struct smb2_file_all_info *buf,
> > -         struct smb2_err_rsp **err_buf)
> > +         struct kvec *err_iov)
> >  {
> >         struct smb2_create_req *req;
> >         struct smb2_create_rsp *rsp;
> > @@ -1841,9 +1841,10 @@ SMB2_open(const unsigned int xid, struct
> > cifs_open_parms *oparms, __le16 *path,
> >
> >         if (rc != 0) {
> >                 cifs_stats_fail_inc(tcon, SMB2_CREATE_HE);
> > -               if (err_buf && rsp)
> > -                       *err_buf = kmemdup(rsp, get_rfc1002_length(rsp) +
> > 4,
> > -                                          GFP_KERNEL);
> > +               if (err_iov && rsp) {
> > +                       *err_iov = rsp_iov;
> > +                       rsp = NULL;
> 
> Should we also change a resp_buf_type here to CIFS_NO_BUFFER? This
> will prevent cifs_small_buf_release() to complain that a null pointer
> is passed.

Yes that is a good idea.
Steve, can you fixup this patch when you merge, or do you want me to re-send it ?


> 
> > +               }
> >                 goto creat_exit;
> >         }
> >
> > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> > index cbcce3f7e86f..8ba24a95db71 100644
> > --- a/fs/cifs/smb2proto.h
> > +++ b/fs/cifs/smb2proto.h
> > @@ -122,7 +122,7 @@ extern int SMB2_tdis(const unsigned int xid, struct
> > cifs_tcon *tcon);
> >  extern int SMB2_open(const unsigned int xid, struct cifs_open_parms
> >  *oparms,
> >                      __le16 *path, __u8 *oplock,
> >                      struct smb2_file_all_info *buf,
> > -                    struct smb2_err_rsp **err_buf);
> > +                    struct kvec *err_iov);
> >  extern int SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon,
> >                      u64 persistent_fid, u64 volatile_fid, u32 opcode,
> >                      bool is_fsctl, char *in_data, u32 indatalen,
> > --
> > 2.13.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> Other than the notice above, the patch looks good.
> 
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> 
> --
> Best regards,
> Pavel Shilovsky
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index def84fed3571..b4ae932ea134 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1451,6 +1451,7 @@  smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
 	__u8 oplock = SMB2_OPLOCK_LEVEL_NONE;
 	struct cifs_open_parms oparms;
 	struct cifs_fid fid;
+	struct kvec err_iov = {NULL, 0};
 	struct smb2_err_rsp *err_buf = NULL;
 	struct smb2_symlink_err_rsp *symlink;
 	unsigned int sub_len;
@@ -1473,15 +1474,16 @@  smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
 	oparms.fid = &fid;
 	oparms.reconnect = false;
 
-	rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, &err_buf);
+	rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, &err_iov);
 
 	if (!rc || !err_buf) {
 		kfree(utf16_path);
 		return -ENOENT;
 	}
 
+	err_buf = err_iov.iov_base;
 	if (le32_to_cpu(err_buf->ByteCount) < sizeof(struct smb2_symlink_err_rsp) ||
-	    get_rfc1002_length(err_buf) + server->vals->header_preamble_size < SMB2_SYMLINK_STRUCT_SIZE) {
+	    err_iov.iov_len + server->vals->header_preamble_size < SMB2_SYMLINK_STRUCT_SIZE) {
 		kfree(utf16_path);
 		return -ENOENT;
 	}
@@ -1494,13 +1496,13 @@  smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
 	print_len = le16_to_cpu(symlink->PrintNameLength);
 	print_offset = le16_to_cpu(symlink->PrintNameOffset);
 
-	if (get_rfc1002_length(err_buf) + server->vals->header_preamble_size <
+	if (err_iov.iov_len + server->vals->header_preamble_size <
 			SMB2_SYMLINK_STRUCT_SIZE + sub_offset + sub_len) {
 		kfree(utf16_path);
 		return -ENOENT;
 	}
 
-	if (get_rfc1002_length(err_buf) + server->vals->header_preamble_size <
+	if (err_iov.iov_len + server->vals->header_preamble_size <
 			SMB2_SYMLINK_STRUCT_SIZE + print_offset + print_len) {
 		kfree(utf16_path);
 		return -ENOENT;
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 30dade197e7e..dc66b93131f3 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1701,7 +1701,7 @@  alloc_path_with_tree_prefix(__le16 **out_path, int *out_size, int *out_len,
 int
 SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
 	  __u8 *oplock, struct smb2_file_all_info *buf,
-	  struct smb2_err_rsp **err_buf)
+	  struct kvec *err_iov)
 {
 	struct smb2_create_req *req;
 	struct smb2_create_rsp *rsp;
@@ -1841,9 +1841,10 @@  SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
 
 	if (rc != 0) {
 		cifs_stats_fail_inc(tcon, SMB2_CREATE_HE);
-		if (err_buf && rsp)
-			*err_buf = kmemdup(rsp, get_rfc1002_length(rsp) + 4,
-					   GFP_KERNEL);
+		if (err_iov && rsp) {
+			*err_iov = rsp_iov;
+			rsp = NULL;
+		}
 		goto creat_exit;
 	}
 
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index cbcce3f7e86f..8ba24a95db71 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -122,7 +122,7 @@  extern int SMB2_tdis(const unsigned int xid, struct cifs_tcon *tcon);
 extern int SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms,
 		     __le16 *path, __u8 *oplock,
 		     struct smb2_file_all_info *buf,
-		     struct smb2_err_rsp **err_buf);
+		     struct kvec *err_iov);
 extern int SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon,
 		     u64 persistent_fid, u64 volatile_fid, u32 opcode,
 		     bool is_fsctl, char *in_data, u32 indatalen,