I think it is ok. We allocate path but assign it to the iov. (which is rqst->rq_iov)
This is later kfree()d in SMB2_open_free()
----- Original Message -----
> From: "Aurélien Aptel" <aaptel@xxxxxxxx>
> To: "Ronnie Sahlberg" <lsahlber@xxxxxxxxxx>, "linux-cifs" <linux-cifs@xxxxxxxxxxxxxxx>
> Cc: "Steve French" <smfrench@xxxxxxxxx>
> Sent: Tuesday, 31 July, 2018 9:41:08 PM
> Subject: Re: [PATCH 5/8] cifs: create SMB2_open_init()/SMB2_open_free() helpers.
>
> I think there's a leak here:
>
> Ronnie Sahlberg <lsahlber@xxxxxxxxxx> writes:
> > + copy_size = uni_path_len;
> > + if (copy_size % 8 != 0)
> > + copy_size = roundup(copy_size, 8);
> > + copy_path = kzalloc(copy_size, GFP_KERNEL);
> > + if (!copy_path)
> > + return -ENOMEM;
> > + memcpy((char *)copy_path, (const char *)path,
> > + uni_path_len);
> > + uni_path_len = copy_size;
> > + path = copy_path;
>
> We still allocate the paths
>
> > @@ -2163,12 +2138,8 @@ SMB2_open(const unsigned int xid, struct
> > cifs_open_parms *oparms, __le16 *path,
> > else {
> > rc = add_lease_context(server, iov, &n_iov,
> > oparms->fid->lease_key, oplock);
> > - if (rc) {
> > - cifs_small_buf_release(req);
> > - kfree(copy_path);
> > + if (rc)
> > return rc;
> > - }
> > - lc_buf = iov[n_iov-1].iov_base;
> > }
> >
> > if (*oplock == SMB2_OPLOCK_LEVEL_BATCH) {
> > @@ -2182,13 +2153,8 @@ SMB2_open(const unsigned int xid, struct
> > cifs_open_parms *oparms, __le16 *path,
> >
> > rc = add_durable_context(iov, &n_iov, oparms,
> > tcon->use_persistent);
> > - if (rc) {
> > - cifs_small_buf_release(req);
> > - kfree(copy_path);
> > - kfree(lc_buf);
> > + if (rc)
> > return rc;
> > - }
> > - dhc_buf = iov[n_iov-1].iov_base;
> > }
> >
> > if (tcon->posix_extensions) {
> > @@ -2200,23 +2166,63 @@ SMB2_open(const unsigned int xid, struct
> > cifs_open_parms *oparms, __le16 *path,
> > }
> >
> > rc = add_posix_context(iov, &n_iov, oparms->mode);
> > - if (rc) {
> > - cifs_small_buf_release(req);
> > - kfree(copy_path);
> > - kfree(lc_buf);
> > - kfree(dhc_buf);
> > + if (rc)
> > return rc;
> > - }
> > - pc_buf = iov[n_iov-1].iov_base;
> > }
> >
> > + rqst->rq_nvec = n_iov;
> > + return 0;
> > +}
> > +
> > +/* rq_iov[0] is the request and is released by cifs_small_buf_release().
> > + * All other vectors are freed by kfree().
> > + */
> > +void
> > +SMB2_open_free(struct smb_rqst *rqst) {
> > + int i;
> > +
> > + cifs_small_buf_release(rqst->rq_iov[0].iov_base);
> > + for (i = 1; i < rqst->rq_nvec; i++)
> > + kfree(rqst->rq_iov[i].iov_base);
> > +}
>
> But it seems we never free them now
>
>
> > @@ -2253,10 +2259,7 @@ SMB2_open(const unsigned int xid, struct
> > cifs_open_parms *oparms, __le16 *path,
> > else
> > *oplock = rsp->OplockLevel;
> > creat_exit:
> > - kfree(copy_path);
> > - kfree(lc_buf);
> > - kfree(dhc_buf);
> > - kfree(pc_buf);
> > + SMB2_open_free(&rqst);
> > free_rsp_buf(resp_buftype, rsp);
> > return rc;
> > }
>
> Not here either.
>
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
> SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
