| Message ID | 49EF1F30.1030308@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 22 Apr 2009 19:14:16 +0530 Suresh Jayaraman <sjayaraman@suse.de> wrote: > Move the memory allocation from cifs_follow_link to > CIFSSMBUnixQuerySymLink and make use of cifs_strlcpy_to_host(). Also > cleaned up a bit while at it. > > Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de> > --- > fs/cifs/cifssmb.c | 23 +++++++---------------- > fs/cifs/link.c | 23 ++++++----------------- > 2 files changed, 13 insertions(+), 33 deletions(-) > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > index a02c43b..b5deb47 100644 > --- a/fs/cifs/cifssmb.c > +++ b/fs/cifs/cifssmb.c > @@ -2488,24 +2488,15 @@ querySymLinkRetry: > else { > __u16 data_offset = le16_to_cpu(pSMBr->t2.DataOffset); > __u16 count = le16_to_cpu(pSMBr->t2.DataCount); > + char *src = (char *) &pSMBr->hdr.Protocol + data_offset; > + int src_len = min_t(const int, buflen, count); > > if (pSMBr->hdr.Flags2 & SMBFLG2_UNICODE) { > - name_len = UniStrnlen((wchar_t *) ((char *) > - &pSMBr->hdr.Protocol + data_offset), > - min_t(const int, buflen, count) / 2); > - /* BB FIXME investigate remapping reserved chars here */ > - cifs_strfromUCS_le(symlinkinfo, > - (__le16 *) ((char *)&pSMBr->hdr.Protocol > - + data_offset), > - name_len, nls_codepage); > - } else { > - strncpy(symlinkinfo, > - (char *) &pSMBr->hdr.Protocol + > - data_offset, > - min_t(const int, buflen, count)); > - } > - symlinkinfo[buflen] = 0; > - /* just in case so calling code does not go off the end of buffer */ > + rc = cifs_strlcpy_to_host(&symlinkinfo, src, > + src_len / 2, 1, nls_codepage); > + cFYI(1, ("symlinkinfo = %s", symlinkinfo)); > + } else > + strlcpy(symlinkinfo, src, src_len + 1); > } > } > cifs_buf_release(pSMB); > diff --git a/fs/cifs/link.c b/fs/cifs/link.c > index 63f6440..5c1d87c 100644 > --- a/fs/cifs/link.c > +++ b/fs/cifs/link.c > @@ -124,11 +124,6 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd) > cFYI(1, ("Full path: %s inode = 0x%p", full_path, inode)); > cifs_sb = CIFS_SB(inode->i_sb); > pTcon = cifs_sb->tcon; > - target_path = kmalloc(PATH_MAX, GFP_KERNEL); > - if (!target_path) { > - target_path = ERR_PTR(-ENOMEM); > - goto out; > - } > > /* We could change this to: > if (pTcon->unix_ext) > @@ -136,11 +131,16 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd) > get symlink info if we can, even if unix extensions > turned off for this mount */ > > - if (pTcon->ses->capabilities & CAP_UNIX) > + if (pTcon->ses->capabilities & CAP_UNIX) { > rc = CIFSSMBUnixQuerySymLink(xid, pTcon, full_path, > target_path, > PATH_MAX-1, > cifs_sb->local_nls); > + if (rc) { > + kfree(target_path); > + target_path = ERR_PTR(rc); > + } > + } Ummmm....that won't work. You're now allocating the buffer in CIFSSMBUnixQuerySymLink, but target_path is just a normal char pointer. There's no way to pass the pointer back to the caller unless you change the args for CIFSSMBUnixQuerySymLink. Please make sure you test these patches before sending them to the list. > else { > /* BB add read reparse point symlink code here */ > /* rc = CIFSSMBQueryReparseLinkInfo */ > @@ -148,17 +148,6 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd) > /* BB Add MAC style xsymlink check here if enabled */ > } > > - if (rc == 0) { > - > -/* BB Add special case check for Samba DFS symlinks */ > - > - target_path[PATH_MAX-1] = 0; > - } else { > - kfree(target_path); > - target_path = ERR_PTR(rc); > - } > - > -out: > kfree(full_path); > out_no_free: > FreeXid(xid);
Jeff Layton wrote: > On Wed, 22 Apr 2009 19:14:16 +0530 > Suresh Jayaraman <sjayaraman@suse.de> wrote: > >> Move the memory allocation from cifs_follow_link to >> CIFSSMBUnixQuerySymLink and make use of cifs_strlcpy_to_host(). Also >> cleaned up a bit while at it. >> >> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de> >> --- >> fs/cifs/cifssmb.c | 23 +++++++---------------- >> fs/cifs/link.c | 23 ++++++----------------- >> 2 files changed, 13 insertions(+), 33 deletions(-) >> >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c >> index a02c43b..b5deb47 100644 >> --- a/fs/cifs/cifssmb.c >> +++ b/fs/cifs/cifssmb.c >> @@ -2488,24 +2488,15 @@ querySymLinkRetry: >> else { >> __u16 data_offset = le16_to_cpu(pSMBr->t2.DataOffset); >> __u16 count = le16_to_cpu(pSMBr->t2.DataCount); >> + char *src = (char *) &pSMBr->hdr.Protocol + data_offset; >> + int src_len = min_t(const int, buflen, count); >> >> if (pSMBr->hdr.Flags2 & SMBFLG2_UNICODE) { >> - name_len = UniStrnlen((wchar_t *) ((char *) >> - &pSMBr->hdr.Protocol + data_offset), >> - min_t(const int, buflen, count) / 2); >> - /* BB FIXME investigate remapping reserved chars here */ >> - cifs_strfromUCS_le(symlinkinfo, >> - (__le16 *) ((char *)&pSMBr->hdr.Protocol >> - + data_offset), >> - name_len, nls_codepage); >> - } else { >> - strncpy(symlinkinfo, >> - (char *) &pSMBr->hdr.Protocol + >> - data_offset, >> - min_t(const int, buflen, count)); >> - } >> - symlinkinfo[buflen] = 0; >> - /* just in case so calling code does not go off the end of buffer */ >> + rc = cifs_strlcpy_to_host(&symlinkinfo, src, >> + src_len / 2, 1, nls_codepage); >> + cFYI(1, ("symlinkinfo = %s", symlinkinfo)); >> + } else >> + strlcpy(symlinkinfo, src, src_len + 1); >> } >> } >> cifs_buf_release(pSMB); >> diff --git a/fs/cifs/link.c b/fs/cifs/link.c >> index 63f6440..5c1d87c 100644 >> --- a/fs/cifs/link.c >> +++ b/fs/cifs/link.c >> @@ -124,11 +124,6 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd) >> cFYI(1, ("Full path: %s inode = 0x%p", full_path, inode)); >> cifs_sb = CIFS_SB(inode->i_sb); >> pTcon = cifs_sb->tcon; >> - target_path = kmalloc(PATH_MAX, GFP_KERNEL); >> - if (!target_path) { >> - target_path = ERR_PTR(-ENOMEM); >> - goto out; >> - } >> >> /* We could change this to: >> if (pTcon->unix_ext) >> @@ -136,11 +131,16 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd) >> get symlink info if we can, even if unix extensions >> turned off for this mount */ >> >> - if (pTcon->ses->capabilities & CAP_UNIX) >> + if (pTcon->ses->capabilities & CAP_UNIX) { >> rc = CIFSSMBUnixQuerySymLink(xid, pTcon, full_path, >> target_path, ^^^^^^^^^^ It's already passed to CIFSSMBUnixQuerySymLink(). Now only the allocation is done in CIFSSMBUnixQuerySymLink() PATH_MAX-1, >> cifs_sb->local_nls); >> + if (rc) { >> + kfree(target_path); >> + target_path = ERR_PTR(rc); >> + } >> + } > > > Ummmm....that won't work. You're now allocating the buffer in > CIFSSMBUnixQuerySymLink, but target_path is just a normal char pointer. > There's no way to pass the pointer back to the caller unless you change > the args for CIFSSMBUnixQuerySymLink. Did I miss something ? > Please make sure you test these patches before sending them to the list. I must have been in a hurry, Sorry, except for UniStrlenBytes patch the others are only compile tested as the changes seemed trivial.. >> else { >> /* BB add read reparse point symlink code here */ >> /* rc = CIFSSMBQueryReparseLinkInfo */ >> @@ -148,17 +148,6 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd) >> /* BB Add MAC style xsymlink check here if enabled */ >> } >> >> - if (rc == 0) { >> - >> -/* BB Add special case check for Samba DFS symlinks */ >> - >> - target_path[PATH_MAX-1] = 0; >> - } else { >> - kfree(target_path); >> - target_path = ERR_PTR(rc); >> - } >> - >> -out: >> kfree(full_path); >> out_no_free: >> FreeXid(xid); > >
Jeff Layton wrote: > On Wed, 22 Apr 2009 19:14:16 +0530 > Suresh Jayaraman <sjayaraman@suse.de> wrote: > >> Move the memory allocation from cifs_follow_link to >> CIFSSMBUnixQuerySymLink and make use of cifs_strlcpy_to_host(). Also >> cleaned up a bit while at it. >> >> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de> >> --- >> fs/cifs/cifssmb.c | 23 +++++++---------------- >> fs/cifs/link.c | 23 ++++++----------------- >> 2 files changed, 13 insertions(+), 33 deletions(-) >> >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c >> index a02c43b..b5deb47 100644 >> --- a/fs/cifs/cifssmb.c >> +++ b/fs/cifs/cifssmb.c >> @@ -2488,24 +2488,15 @@ querySymLinkRetry: >> else { >> __u16 data_offset = le16_to_cpu(pSMBr->t2.DataOffset); >> __u16 count = le16_to_cpu(pSMBr->t2.DataCount); >> + char *src = (char *) &pSMBr->hdr.Protocol + data_offset; >> + int src_len = min_t(const int, buflen, count); >> >> if (pSMBr->hdr.Flags2 & SMBFLG2_UNICODE) { >> - name_len = UniStrnlen((wchar_t *) ((char *) >> - &pSMBr->hdr.Protocol + data_offset), >> - min_t(const int, buflen, count) / 2); >> - /* BB FIXME investigate remapping reserved chars here */ >> - cifs_strfromUCS_le(symlinkinfo, >> - (__le16 *) ((char *)&pSMBr->hdr.Protocol >> - + data_offset), >> - name_len, nls_codepage); >> - } else { >> - strncpy(symlinkinfo, >> - (char *) &pSMBr->hdr.Protocol + >> - data_offset, >> - min_t(const int, buflen, count)); >> - } >> - symlinkinfo[buflen] = 0; >> - /* just in case so calling code does not go off the end of buffer */ >> + rc = cifs_strlcpy_to_host(&symlinkinfo, src, >> + src_len / 2, 1, nls_codepage); >> + cFYI(1, ("symlinkinfo = %s", symlinkinfo)); >> + } else >> + strlcpy(symlinkinfo, src, src_len + 1); >> } >> } >> cifs_buf_release(pSMB); >> diff --git a/fs/cifs/link.c b/fs/cifs/link.c >> index 63f6440..5c1d87c 100644 >> --- a/fs/cifs/link.c >> +++ b/fs/cifs/link.c >> @@ -124,11 +124,6 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd) >> cFYI(1, ("Full path: %s inode = 0x%p", full_path, inode)); >> cifs_sb = CIFS_SB(inode->i_sb); >> pTcon = cifs_sb->tcon; >> - target_path = kmalloc(PATH_MAX, GFP_KERNEL); >> - if (!target_path) { >> - target_path = ERR_PTR(-ENOMEM); >> - goto out; >> - } >> >> /* We could change this to: >> if (pTcon->unix_ext) >> @@ -136,11 +131,16 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd) >> get symlink info if we can, even if unix extensions >> turned off for this mount */ >> >> - if (pTcon->ses->capabilities & CAP_UNIX) >> + if (pTcon->ses->capabilities & CAP_UNIX) { >> rc = CIFSSMBUnixQuerySymLink(xid, pTcon, full_path, >> target_path, >> PATH_MAX-1, >> cifs_sb->local_nls); >> + if (rc) { >> + kfree(target_path); >> + target_path = ERR_PTR(rc); >> + } >> + } > > > Ummmm....that won't work. You're now allocating the buffer in > CIFSSMBUnixQuerySymLink, but target_path is just a normal char pointer. > There's no way to pass the pointer back to the caller unless you change > the args for CIFSSMBUnixQuerySymLink. oh, stupid me.. I see what you mean I should have passed &target_path and used char **symlinkinfo in CIFSSMBUnixQuerySymLink. Sorry, Ignore this one.. > Please make sure you test these patches before sending them to the list. > >> else { >> /* BB add read reparse point symlink code here */ >> /* rc = CIFSSMBQueryReparseLinkInfo */ >> @@ -148,17 +148,6 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd) >> /* BB Add MAC style xsymlink check here if enabled */ >> } >> >> - if (rc == 0) { >> - >> -/* BB Add special case check for Samba DFS symlinks */ >> - >> - target_path[PATH_MAX-1] = 0; >> - } else { >> - kfree(target_path); >> - target_path = ERR_PTR(rc); >> - } >> - >> -out: >> kfree(full_path); >> out_no_free: >> FreeXid(xid); > >
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index a02c43b..b5deb47 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -2488,24 +2488,15 @@ querySymLinkRetry: else { __u16 data_offset = le16_to_cpu(pSMBr->t2.DataOffset); __u16 count = le16_to_cpu(pSMBr->t2.DataCount); + char *src = (char *) &pSMBr->hdr.Protocol + data_offset; + int src_len = min_t(const int, buflen, count); if (pSMBr->hdr.Flags2 & SMBFLG2_UNICODE) { - name_len = UniStrnlen((wchar_t *) ((char *) - &pSMBr->hdr.Protocol + data_offset), - min_t(const int, buflen, count) / 2); - /* BB FIXME investigate remapping reserved chars here */ - cifs_strfromUCS_le(symlinkinfo, - (__le16 *) ((char *)&pSMBr->hdr.Protocol - + data_offset), - name_len, nls_codepage); - } else { - strncpy(symlinkinfo, - (char *) &pSMBr->hdr.Protocol + - data_offset, - min_t(const int, buflen, count)); - } - symlinkinfo[buflen] = 0; - /* just in case so calling code does not go off the end of buffer */ + rc = cifs_strlcpy_to_host(&symlinkinfo, src, + src_len / 2, 1, nls_codepage); + cFYI(1, ("symlinkinfo = %s", symlinkinfo)); + } else + strlcpy(symlinkinfo, src, src_len + 1); } } cifs_buf_release(pSMB); diff --git a/fs/cifs/link.c b/fs/cifs/link.c index 63f6440..5c1d87c 100644 --- a/fs/cifs/link.c +++ b/fs/cifs/link.c @@ -124,11 +124,6 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd) cFYI(1, ("Full path: %s inode = 0x%p", full_path, inode)); cifs_sb = CIFS_SB(inode->i_sb); pTcon = cifs_sb->tcon; - target_path = kmalloc(PATH_MAX, GFP_KERNEL); - if (!target_path) { - target_path = ERR_PTR(-ENOMEM); - goto out; - } /* We could change this to: if (pTcon->unix_ext) @@ -136,11 +131,16 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd) get symlink info if we can, even if unix extensions turned off for this mount */ - if (pTcon->ses->capabilities & CAP_UNIX) + if (pTcon->ses->capabilities & CAP_UNIX) { rc = CIFSSMBUnixQuerySymLink(xid, pTcon, full_path, target_path, PATH_MAX-1, cifs_sb->local_nls); + if (rc) { + kfree(target_path); + target_path = ERR_PTR(rc); + } + } else { /* BB add read reparse point symlink code here */ /* rc = CIFSSMBQueryReparseLinkInfo */ @@ -148,17 +148,6 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd) /* BB Add MAC style xsymlink check here if enabled */ } - if (rc == 0) { - -/* BB Add special case check for Samba DFS symlinks */ - - target_path[PATH_MAX-1] = 0; - } else { - kfree(target_path); - target_path = ERR_PTR(rc); - } - -out: kfree(full_path); out_no_free: FreeXid(xid);
Move the memory allocation from cifs_follow_link to CIFSSMBUnixQuerySymLink and make use of cifs_strlcpy_to_host(). Also cleaned up a bit while at it. Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de> --- fs/cifs/cifssmb.c | 23 +++++++---------------- fs/cifs/link.c | 23 ++++++----------------- 2 files changed, 13 insertions(+), 33 deletions(-)