linux-kernel - Re: [Patch v2 3/6] cifs: smbd: Avoid allocating iov on the stack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 23 Apr 2018 10:31:51 -0500
From:   Steve French <smfrench@...il.com>
To:     Long Li <longli@...rosoft.com>
Cc:     CIFS <linux-cifs@...r.kernel.org>,
        samba-technical <samba-technical@...ts.samba.org>,
        LKML <linux-kernel@...r.kernel.org>, linux-rdma@...r.kernel.org
Subject: Re: [Patch v2 3/6] cifs: smbd: Avoid allocating iov on the stack

Didn't see any obvious problems, but can you fix the checkpatch
warnings and resend to the list (I am more concerned about the last
two warnings rather than the first one).

$ scripts/checkpatch.pl 0001-cifs-smbd-Avoid-allocating-iov-on-the-stack.patch
WARNING: line over 80 characters
#60: FILE: fs/cifs/smbdirect.c:2106:
+        log_write(ERR, "expected the pdu length in 1st iov, but got
0x%lu\n", rqst->rq_iov[0].iov_len);

ERROR: Prefixing 0x with decimal output is defective
#60: FILE: fs/cifs/smbdirect.c:2106:
+        log_write(ERR, "expected the pdu length in 1st iov, but got
0x%lu\n", rqst->rq_iov[0].iov_len);

WARNING: braces {} are not necessary for single statement blocks
#69: FILE: fs/cifs/smbdirect.c:2112:
+    for (i = 0; i < rqst->rq_nvec-1; i++) {
         buflen += iov[i].iov_len;
     }

total: 1 errors, 2 warnings, 65 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0001-cifs-smbd-Avoid-allocating-iov-on-the-stack.patch has style
problems, please review.

On Tue, Apr 17, 2018 at 2:17 PM, Long Li <longli@...uxonhyperv.com> wrote:
> From: Long Li <longli@...rosoft.com>
>
> It's not necessary to allocate another iov when going through the buffers
> in smbd_send() through RDMA send.
>
> Remove it to reduce stack size.
>
> Signed-off-by: Long Li <longli@...rosoft.com>
> Cc: stable@...r.kernel.org
> ---
>  fs/cifs/smbdirect.c | 36 ++++++++++++------------------------
>  1 file changed, 12 insertions(+), 24 deletions(-)
>
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index b5c6c0d..f575e9a 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -2088,7 +2088,7 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
>         int start, i, j;
>         int max_iov_size =
>                 info->max_send_size - sizeof(struct smbd_data_transfer);
> -       struct kvec iov[SMBDIRECT_MAX_SGE];
> +       struct kvec *iov;
>         int rc;
>
>         info->smbd_send_pending++;
> @@ -2099,32 +2099,20 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
>         }
>
>         /*
> -        * This usually means a configuration error
> -        * We use RDMA read/write for packet size > rdma_readwrite_threshold
> -        * as long as it's properly configured we should never get into this
> -        * situation
> -        */
> -       if (rqst->rq_nvec + rqst->rq_npages > SMBDIRECT_MAX_SGE) {
> -               log_write(ERR, "maximum send segment %x exceeding %x\n",
> -                        rqst->rq_nvec + rqst->rq_npages, SMBDIRECT_MAX_SGE);
> -               rc = -EINVAL;
> -               goto done;
> -       }
> -
> -       /*
> -        * Remove the RFC1002 length defined in MS-SMB2 section 2.1
> -        * It is used only for TCP transport
> +        * Skip the RFC1002 length defined in MS-SMB2 section 2.1
> +        * It is used only for TCP transport in the iov[0]
>          * In future we may want to add a transport layer under protocol
>          * layer so this will only be issued to TCP transport
>          */
> -       iov[0].iov_base = (char *)rqst->rq_iov[0].iov_base + 4;
> -       iov[0].iov_len = rqst->rq_iov[0].iov_len - 4;
> -       buflen += iov[0].iov_len;
> +
> +       if (rqst->rq_iov[0].iov_len != 4) {
> +               log_write(ERR, "expected the pdu length in 1st iov, but got 0x%lu\n", rqst->rq_iov[0].iov_len);
> +               return -EINVAL;
> +       }
> +       iov = &rqst->rq_iov[1];
>
>         /* total up iov array first */
> -       for (i = 1; i < rqst->rq_nvec; i++) {
> -               iov[i].iov_base = rqst->rq_iov[i].iov_base;
> -               iov[i].iov_len = rqst->rq_iov[i].iov_len;
> +       for (i = 0; i < rqst->rq_nvec-1; i++) {
>                 buflen += iov[i].iov_len;
>         }
>
> @@ -2197,14 +2185,14 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
>                                                 goto done;
>                                 }
>                                 i++;
> -                               if (i == rqst->rq_nvec)
> +                               if (i == rqst->rq_nvec-1)
>                                         break;
>                         }
>                         start = i;
>                         buflen = 0;
>                 } else {
>                         i++;
> -                       if (i == rqst->rq_nvec) {
> +                       if (i == rqst->rq_nvec-1) {
>                                 /* send out all remaining vecs */
>                                 remaining_data_length -= buflen;
>                                 log_write(INFO,
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve