linux-kernel - RE: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer to send data

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]

Message-ID: <MWHPR21MB0190DC9FE518F018FEE011ADCE810@MWHPR21MB0190.namprd21.prod.outlook.com>
Date:   Sat, 19 Aug 2017 23:41:45 +0000
From:   Long Li <longli@...rosoft.com>
To:     Tom Talpey <ttalpey@...rosoft.com>,
        Steve French <sfrench@...ba.org>,
        "linux-cifs@...r.kernel.org" <linux-cifs@...r.kernel.org>,
        "samba-technical@...ts.samba.org" <samba-technical@...ts.samba.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>
Subject: RE: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer to
 send data



> -----Original Message-----
> From: Tom Talpey
> Sent: Monday, August 14, 2017 1:44 PM
> To: Long Li <longli@...rosoft.com>; Steve French <sfrench@...ba.org>;
> linux-cifs@...r.kernel.org; samba-technical@...ts.samba.org; linux-
> kernel@...r.kernel.org; linux-rdma@...r.kernel.org
> Subject: RE: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer
> to send data
> 
> > -----Original Message-----
> > From: linux-cifs-owner@...r.kernel.org [mailto:linux-cifs-
> > owner@...r.kernel.org] On Behalf Of Long Li
> > Sent: Wednesday, August 2, 2017 4:10 PM
> > To: Steve French <sfrench@...ba.org>; linux-cifs@...r.kernel.org;
> > samba- technical@...ts.samba.org; linux-kernel@...r.kernel.org
> > Cc: Long Li <longli@...rosoft.com>
> > Subject: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer
> > to send data
> >
> > +/*
> > + * Write data to transport
> > + * Each rqst is transported as a SMBDirect payload
> > + * rqst: the data to write
> > + * return value: 0 if successfully write, otherwise error code  */
> > +int cifs_rdma_write(struct cifs_rdma_info *info, struct smb_rqst
> > +*rqst) {
> 
> !!!
> This is a VERY confusing name. It is not sending an RDMA Write, which will
> confuse any RDMA-enlightened reader. It's performing an RDMA Send, so
> that name is perhaps one possibility.
> 
> > +       if (info->transport_status != CIFS_RDMA_CONNECTED) {
> > +               log_cifs_write("disconnected returning -EIO\n");
> > +               return -EIO;
> > +       }
> 
> Isn't this optimizing the error case? There's no guarantee it's still connected
> by the time the following request construction occurs. Why not just proceed
> without the check?
> 
> > +       /* Strip the first 4 bytes MS-SMB2 section 2.1
> > +        * they are used only for 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;
> 
> Ok, that layering choice in the cifs.ko client code needs to be corrected. After
> all, it will need to be RDMA-aware to build the SMB3 read/write channel
> structures.
> And, the code in cifs_post_send_data() is allocating and building a structure
> that could have been accounted for much earlier, avoiding the extra
> overhead.
> 
> That change could happen later, the hack is mostly ok for now. But
> something needs to be said in a comment.

Will address those in v2.

> 
> Tom.