diff mbox series

[7/8] smb: client: reduce stack usage in smb2_query_symlink()

Message ID 20230728195010.19122-7-pc@manguebit.com (mailing list archive)
State New, archived
Headers show
Series [1/8] smb: client: reduce stack usage in cifs_try_adding_channels() | expand

Commit Message

Paulo Alcantara July 28, 2023, 7:50 p.m. UTC 
Clang warns about exceeded stack frame size

  fs/smb/client/smb2ops.c:2974:1: warning: stack frame size (1368)
  exceeds limit (1024) in 'smb2_query_symlink' [-Wframe-larger-than]

Fix this by allocating a qs_vars structure that will hold most of the
large structures.

Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
---
 fs/smb/client/smb2ops.c | 43 ++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 16 deletions(-)

Comments

Enzo Matsumiya July 28, 2023, 8:04 p.m. UTC | #1 
On 07/28, Paulo Alcantara wrote:
>Clang warns about exceeded stack frame size
>
>  fs/smb/client/smb2ops.c:2974:1: warning: stack frame size (1368)
>  exceeds limit (1024) in 'smb2_query_symlink' [-Wframe-larger-than]
>
>Fix this by allocating a qs_vars structure that will hold most of the
>large structures.
>
>Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
>---
> fs/smb/client/smb2ops.c | 43 ++++++++++++++++++++++++++---------------
> 1 file changed, 27 insertions(+), 16 deletions(-)
>
>diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
>index d6a15d5ec4d2..9136c77cd407 100644
>--- a/fs/smb/client/smb2ops.c
>+++ b/fs/smb/client/smb2ops.c
>@@ -2970,6 +2970,14 @@ parse_reparse_point(struct reparse_data_buffer *buf,
> 	}
> }
>
>+struct qs_vars {
>+	struct smb_rqst rqst[3];
>+	struct kvec rsp_iov[3];
>+	struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
>+	struct kvec io_iov[SMB2_IOCTL_IOV_SIZE];
>+	struct kvec close_iov;
>+};

I think structs qs_vars, qr_vars, and qi_vars should be a single "struct
query_vars" instead of having 2 repeated + 1 very similar differently
named structs around.

Then for smb2_query_info_compound() you use only io_iov[0].

And later on, maybe even embed such struct in "struct cop_vars"
(smb2inode.c) to avoid even more duplicate code.

Other than that,

Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>

for this and all the other patches in this series.


Cheers,

Enzo

>+
> static int
> smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
> 		   struct cifs_sb_info *cifs_sb, const char *full_path,
>@@ -2979,16 +2987,14 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
> 	__le16 *utf16_path = NULL;
> 	__u8 oplock = SMB2_OPLOCK_LEVEL_NONE;
> 	struct cifs_open_parms oparms;
>+	struct smb_rqst *rqst;
> 	struct cifs_fid fid;
>+	struct qs_vars *vars;
> 	struct kvec err_iov = {NULL, 0};
>+	struct kvec *rsp_iov;
> 	struct TCP_Server_Info *server = cifs_pick_channel(tcon->ses);
> 	int flags = CIFS_CP_CREATE_CLOSE_OP;
>-	struct smb_rqst rqst[3];
> 	int resp_buftype[3];
>-	struct kvec rsp_iov[3];
>-	struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
>-	struct kvec io_iov[SMB2_IOCTL_IOV_SIZE];
>-	struct kvec close_iov[1];
> 	struct smb2_create_rsp *create_rsp;
> 	struct smb2_ioctl_rsp *ioctl_rsp;
> 	struct reparse_data_buffer *reparse_buf;
>@@ -3002,17 +3008,22 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
> 	if (smb3_encryption_required(tcon))
> 		flags |= CIFS_TRANSFORM_REQ;
>
>-	memset(rqst, 0, sizeof(rqst));
>-	resp_buftype[0] = resp_buftype[1] = resp_buftype[2] = CIFS_NO_BUFFER;
>-	memset(rsp_iov, 0, sizeof(rsp_iov));
>-
> 	utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
> 	if (!utf16_path)
> 		return -ENOMEM;
>
>+	resp_buftype[0] = resp_buftype[1] = resp_buftype[2] = CIFS_NO_BUFFER;
>+
>+	vars = kzalloc(sizeof(*vars), GFP_KERNEL);
>+	if (!vars) {
>+		rc = -ENOMEM;
>+		goto out_free_path;
>+	}
>+	rqst = vars->rqst;
>+	rsp_iov = vars->rsp_iov;
>+
> 	/* Open */
>-	memset(&open_iov, 0, sizeof(open_iov));
>-	rqst[0].rq_iov = open_iov;
>+	rqst[0].rq_iov = vars->open_iov;
> 	rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE;
>
> 	oparms = (struct cifs_open_parms) {
>@@ -3032,8 +3043,7 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
>
>
> 	/* IOCTL */
>-	memset(&io_iov, 0, sizeof(io_iov));
>-	rqst[1].rq_iov = io_iov;
>+	rqst[1].rq_iov = vars->io_iov;
> 	rqst[1].rq_nvec = SMB2_IOCTL_IOV_SIZE;
>
> 	rc = SMB2_ioctl_init(tcon, server,
>@@ -3050,8 +3060,7 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
>
>
> 	/* Close */
>-	memset(&close_iov, 0, sizeof(close_iov));
>-	rqst[2].rq_iov = close_iov;
>+	rqst[2].rq_iov = &vars->close_iov;
> 	rqst[2].rq_nvec = 1;
>
> 	rc = SMB2_close_init(tcon, server,
>@@ -3103,13 +3112,15 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
>
>  querty_exit:
> 	cifs_dbg(FYI, "query symlink rc %d\n", rc);
>-	kfree(utf16_path);
> 	SMB2_open_free(&rqst[0]);
> 	SMB2_ioctl_free(&rqst[1]);
> 	SMB2_close_free(&rqst[2]);
> 	free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
> 	free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> 	free_rsp_buf(resp_buftype[2], rsp_iov[2].iov_base);
>+	kfree(vars);
>+out_free_path:
>+	kfree(utf16_path);
> 	return rc;
> }
>
>-- 
>2.41.0
>
Steve French July 28, 2023, 8:32 p.m. UTC | #2 
probably best to limit additional cleanup (and thus patch size) of
these so they don't get confusing and harder to backport.

We also have lots of testcases (xfstest features and failures) to work
through one by one that are high priority.

The obvious question is - are any of these high enough priority to go
into e.g. rc4 or rc5 - or wait till the merge window?

On Fri, Jul 28, 2023 at 3:04 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> On 07/28, Paulo Alcantara wrote:
> >Clang warns about exceeded stack frame size
> >
> >  fs/smb/client/smb2ops.c:2974:1: warning: stack frame size (1368)
> >  exceeds limit (1024) in 'smb2_query_symlink' [-Wframe-larger-than]
> >
> >Fix this by allocating a qs_vars structure that will hold most of the
> >large structures.
> >
> >Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> >---
> > fs/smb/client/smb2ops.c | 43 ++++++++++++++++++++++++++---------------
> > 1 file changed, 27 insertions(+), 16 deletions(-)
> >
> >diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> >index d6a15d5ec4d2..9136c77cd407 100644
> >--- a/fs/smb/client/smb2ops.c
> >+++ b/fs/smb/client/smb2ops.c
> >@@ -2970,6 +2970,14 @@ parse_reparse_point(struct reparse_data_buffer *buf,
> >       }
> > }
> >
> >+struct qs_vars {
> >+      struct smb_rqst rqst[3];
> >+      struct kvec rsp_iov[3];
> >+      struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
> >+      struct kvec io_iov[SMB2_IOCTL_IOV_SIZE];
> >+      struct kvec close_iov;
> >+};
>
> I think structs qs_vars, qr_vars, and qi_vars should be a single "struct
> query_vars" instead of having 2 repeated + 1 very similar differently
> named structs around.
>
> Then for smb2_query_info_compound() you use only io_iov[0].
>
> And later on, maybe even embed such struct in "struct cop_vars"
> (smb2inode.c) to avoid even more duplicate code.
>
> Other than that,
>
> Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
>
> for this and all the other patches in this series.
>
>
> Cheers,
>
> Enzo
>
> >+
> > static int
> > smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
> >                  struct cifs_sb_info *cifs_sb, const char *full_path,
> >@@ -2979,16 +2987,14 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
> >       __le16 *utf16_path = NULL;
> >       __u8 oplock = SMB2_OPLOCK_LEVEL_NONE;
> >       struct cifs_open_parms oparms;
> >+      struct smb_rqst *rqst;
> >       struct cifs_fid fid;
> >+      struct qs_vars *vars;
> >       struct kvec err_iov = {NULL, 0};
> >+      struct kvec *rsp_iov;
> >       struct TCP_Server_Info *server = cifs_pick_channel(tcon->ses);
> >       int flags = CIFS_CP_CREATE_CLOSE_OP;
> >-      struct smb_rqst rqst[3];
> >       int resp_buftype[3];
> >-      struct kvec rsp_iov[3];
> >-      struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
> >-      struct kvec io_iov[SMB2_IOCTL_IOV_SIZE];
> >-      struct kvec close_iov[1];
> >       struct smb2_create_rsp *create_rsp;
> >       struct smb2_ioctl_rsp *ioctl_rsp;
> >       struct reparse_data_buffer *reparse_buf;
> >@@ -3002,17 +3008,22 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
> >       if (smb3_encryption_required(tcon))
> >               flags |= CIFS_TRANSFORM_REQ;
> >
> >-      memset(rqst, 0, sizeof(rqst));
> >-      resp_buftype[0] = resp_buftype[1] = resp_buftype[2] = CIFS_NO_BUFFER;
> >-      memset(rsp_iov, 0, sizeof(rsp_iov));
> >-
> >       utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
> >       if (!utf16_path)
> >               return -ENOMEM;
> >
> >+      resp_buftype[0] = resp_buftype[1] = resp_buftype[2] = CIFS_NO_BUFFER;
> >+
> >+      vars = kzalloc(sizeof(*vars), GFP_KERNEL);
> >+      if (!vars) {
> >+              rc = -ENOMEM;
> >+              goto out_free_path;
> >+      }
> >+      rqst = vars->rqst;
> >+      rsp_iov = vars->rsp_iov;
> >+
> >       /* Open */
> >-      memset(&open_iov, 0, sizeof(open_iov));
> >-      rqst[0].rq_iov = open_iov;
> >+      rqst[0].rq_iov = vars->open_iov;
> >       rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE;
> >
> >       oparms = (struct cifs_open_parms) {
> >@@ -3032,8 +3043,7 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
> >
> >
> >       /* IOCTL */
> >-      memset(&io_iov, 0, sizeof(io_iov));
> >-      rqst[1].rq_iov = io_iov;
> >+      rqst[1].rq_iov = vars->io_iov;
> >       rqst[1].rq_nvec = SMB2_IOCTL_IOV_SIZE;
> >
> >       rc = SMB2_ioctl_init(tcon, server,
> >@@ -3050,8 +3060,7 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
> >
> >
> >       /* Close */
> >-      memset(&close_iov, 0, sizeof(close_iov));
> >-      rqst[2].rq_iov = close_iov;
> >+      rqst[2].rq_iov = &vars->close_iov;
> >       rqst[2].rq_nvec = 1;
> >
> >       rc = SMB2_close_init(tcon, server,
> >@@ -3103,13 +3112,15 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
> >
> >  querty_exit:
> >       cifs_dbg(FYI, "query symlink rc %d\n", rc);
> >-      kfree(utf16_path);
> >       SMB2_open_free(&rqst[0]);
> >       SMB2_ioctl_free(&rqst[1]);
> >       SMB2_close_free(&rqst[2]);
> >       free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
> >       free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> >       free_rsp_buf(resp_buftype[2], rsp_iov[2].iov_base);
> >+      kfree(vars);
> >+out_free_path:
> >+      kfree(utf16_path);
> >       return rc;
> > }
> >
> >--
> >2.41.0
> >
diff mbox series

Patch

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index d6a15d5ec4d2..9136c77cd407 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -2970,6 +2970,14 @@  parse_reparse_point(struct reparse_data_buffer *buf,
 	}
 }
 
+struct qs_vars {
+	struct smb_rqst rqst[3];
+	struct kvec rsp_iov[3];
+	struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
+	struct kvec io_iov[SMB2_IOCTL_IOV_SIZE];
+	struct kvec close_iov;
+};
+
 static int
 smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
 		   struct cifs_sb_info *cifs_sb, const char *full_path,
@@ -2979,16 +2987,14 @@  smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
 	__le16 *utf16_path = NULL;
 	__u8 oplock = SMB2_OPLOCK_LEVEL_NONE;
 	struct cifs_open_parms oparms;
+	struct smb_rqst *rqst;
 	struct cifs_fid fid;
+	struct qs_vars *vars;
 	struct kvec err_iov = {NULL, 0};
+	struct kvec *rsp_iov;
 	struct TCP_Server_Info *server = cifs_pick_channel(tcon->ses);
 	int flags = CIFS_CP_CREATE_CLOSE_OP;
-	struct smb_rqst rqst[3];
 	int resp_buftype[3];
-	struct kvec rsp_iov[3];
-	struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
-	struct kvec io_iov[SMB2_IOCTL_IOV_SIZE];
-	struct kvec close_iov[1];
 	struct smb2_create_rsp *create_rsp;
 	struct smb2_ioctl_rsp *ioctl_rsp;
 	struct reparse_data_buffer *reparse_buf;
@@ -3002,17 +3008,22 @@  smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
 	if (smb3_encryption_required(tcon))
 		flags |= CIFS_TRANSFORM_REQ;
 
-	memset(rqst, 0, sizeof(rqst));
-	resp_buftype[0] = resp_buftype[1] = resp_buftype[2] = CIFS_NO_BUFFER;
-	memset(rsp_iov, 0, sizeof(rsp_iov));
-
 	utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
 	if (!utf16_path)
 		return -ENOMEM;
 
+	resp_buftype[0] = resp_buftype[1] = resp_buftype[2] = CIFS_NO_BUFFER;
+
+	vars = kzalloc(sizeof(*vars), GFP_KERNEL);
+	if (!vars) {
+		rc = -ENOMEM;
+		goto out_free_path;
+	}
+	rqst = vars->rqst;
+	rsp_iov = vars->rsp_iov;
+
 	/* Open */
-	memset(&open_iov, 0, sizeof(open_iov));
-	rqst[0].rq_iov = open_iov;
+	rqst[0].rq_iov = vars->open_iov;
 	rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE;
 
 	oparms = (struct cifs_open_parms) {
@@ -3032,8 +3043,7 @@  smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
 
 
 	/* IOCTL */
-	memset(&io_iov, 0, sizeof(io_iov));
-	rqst[1].rq_iov = io_iov;
+	rqst[1].rq_iov = vars->io_iov;
 	rqst[1].rq_nvec = SMB2_IOCTL_IOV_SIZE;
 
 	rc = SMB2_ioctl_init(tcon, server,
@@ -3050,8 +3060,7 @@  smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
 
 
 	/* Close */
-	memset(&close_iov, 0, sizeof(close_iov));
-	rqst[2].rq_iov = close_iov;
+	rqst[2].rq_iov = &vars->close_iov;
 	rqst[2].rq_nvec = 1;
 
 	rc = SMB2_close_init(tcon, server,
@@ -3103,13 +3112,15 @@  smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
 
  querty_exit:
 	cifs_dbg(FYI, "query symlink rc %d\n", rc);
-	kfree(utf16_path);
 	SMB2_open_free(&rqst[0]);
 	SMB2_ioctl_free(&rqst[1]);
 	SMB2_close_free(&rqst[2]);
 	free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
 	free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
 	free_rsp_buf(resp_buftype[2], rsp_iov[2].iov_base);
+	kfree(vars);
+out_free_path:
+	kfree(utf16_path);
 	return rc;
 }