On Thu, 2 Aug 2012 01:07:45 -0500
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Mon, Jul 30, 2012 at 6:00 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> > On Wed, 25 Jul 2012 17:45:04 -0500
> > shirishpargaonkar@xxxxxxxxx wrote:
> >
> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >>
> >> path based querries can fail for lack of access, especially during lookup
> >> during open.
> >> open itself would actually succeed becasue of back up intent bit
> >> but querries (either path or file handle based) do not have a means to
> >> specifiy backup intent bit.
> >> So querry the file info during lookup using
> >> trans2 / findfirst2 / file_id_full_ir_info
> >> to obtain file info as well as file_id/inode value.
> >>
> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >> Reported-by: Tushar Gosavi <tugosavi@xxxxxxxxxx>
> >> ---
> >> fs/cifs/cifssmb.c | 38 +++++++++++++++++------------
> >> fs/cifs/inode.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> >> 2 files changed, 87 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> >> index 4ee522b..f059c0a 100644
> >> --- a/fs/cifs/cifssmb.c
> >> +++ b/fs/cifs/cifssmb.c
> >> @@ -4257,7 +4257,7 @@ CIFSFindFirst(const int xid, struct cifs_tcon *tcon,
> >> int rc = 0;
> >> int bytes_returned = 0;
> >> int name_len;
> >> - __u16 params, byte_count;
> >> + __u16 params, byte_count, ffattrs;
> >>
> >> cFYI(1, "In FindFirst for %s", searchName);
> >>
> >> @@ -4275,27 +4275,35 @@ findFirstRetry:
> >> it got remapped to 0xF03A as if it were part of the
> >> directory name instead of a wildcard */
> >> name_len *= 2;
> >> - pSMB->FileName[name_len] = dirsep;
> >> - pSMB->FileName[name_len+1] = 0;
> >> - pSMB->FileName[name_len+2] = '*';
> >> - pSMB->FileName[name_len+3] = 0;
> >> - name_len += 4; /* now the trailing null */
> >> - pSMB->FileName[name_len] = 0; /* null terminate just in case */
> >> - pSMB->FileName[name_len+1] = 0;
> >> - name_len += 2;
> >> + if (dirsep) {
> >
> > This seems like a strange use of the "dirsep" parm. So if you are using
> > this to do a single lookup, then you pass in 0 for dirsep. I guess
> > it'll work, but it's not exactly self-documenting.
>
> How do I otherwise figure out / distinguish that the findfirst operation
> is for a file/leaf and not a directory.
>
> >
> >> + pSMB->FileName[name_len] = dirsep;
> >> + pSMB->FileName[name_len+1] = 0;
> >> + pSMB->FileName[name_len+2] = '*';
> >> + pSMB->FileName[name_len+3] = 0;
> >> + name_len += 4; /* now the trailing null */
> >> + /* null terminate just in case */
> >> + pSMB->FileName[name_len] = 0;
> >> + pSMB->FileName[name_len+1] = 0;
> >> + name_len += 2;
> >> + }
> >> } else { /* BB add check for overrun of SMB buf BB */
> >> name_len = strnlen(searchName, PATH_MAX);
> >> /* BB fix here and in unicode clause above ie
> >> if (name_len > buffersize-header)
> >> free buffer exit; BB */
> >> strncpy(pSMB->FileName, searchName, name_len);
> >> - pSMB->FileName[name_len] = dirsep;
> >> - pSMB->FileName[name_len+1] = '*';
> >> - pSMB->FileName[name_len+2] = 0;
> >> - name_len += 3;
> >> + if (dirsep) {
> >> + pSMB->FileName[name_len] = dirsep;
> >> + pSMB->FileName[name_len+1] = '*';
> >> + pSMB->FileName[name_len+2] = 0;
> >> + name_len += 3;
> >> + }
> >> }
> >>
> >> params = 12 + name_len /* includes null */ ;
> >> + ffattrs = ATTR_READONLY | ATTR_HIDDEN | ATTR_SYSTEM;
> >> + if (dirsep)
> >> + ffattrs |= ATTR_DIRECTORY;
> > ^^^
> > I don't understand this. You're only setting ATTR_DIRECTORY for a readdir()? Why?
>
> Because what I see is, trans2/qpathinfo does not fail on directory, only
> does so for a file. So if dirsep is 0, we are looking for a file and dirsep
> is not 0, findfirst is for a directory.
>
I'm not sure I understand that logic. In fact, I'm not sure I
understand the semantics behind the use of ATTR_DIRECTORY here at all.
What if you're doing a lookup on directory? Does that mean that it will
now be excluded?
MS-CIFS says:
"SearchAttributes (2 bytes): File attributes to apply as a constraint to
the file search. Exclusive search attributes (see section 2.2.1.2.4)
can also be set."
...which makes it sound like you're constraining the search only to
directories when dirsep is set.
If it's necessary to do the above, then a comment explaining why would
be best.
Eventually, I think we should consider using this for all lookups
instead of QPathInfo if we can. In the event that you are using server
inode numbers, this could cut down round trips to the server by half.
Currently we have to do a separate CIFSGetSrvInodeNumber call on every
lookup. It would be nice if we could turn that into a single call.
> >
> >> pSMB->TotalDataCount = 0; /* no EAs */
> >> pSMB->MaxParameterCount = cpu_to_le16(10);
> >> pSMB->MaxDataCount = cpu_to_le16(CIFSMaxBufSize & 0xFFFFFF00);
> >> @@ -4315,9 +4323,7 @@ findFirstRetry:
> >> pSMB->SetupCount = 1; /* one byte, no need to make endian neutral */
> >> pSMB->Reserved3 = 0;
> >> pSMB->SubCommand = cpu_to_le16(TRANS2_FIND_FIRST);
> >> - pSMB->SearchAttributes =
> >> - cpu_to_le16(ATTR_READONLY | ATTR_HIDDEN | ATTR_SYSTEM |
> >> - ATTR_DIRECTORY);
> >> + pSMB->SearchAttributes = cpu_to_le16(ffattrs);
> >> pSMB->SearchCount = cpu_to_le16(CIFSMaxBufSize/sizeof(FILE_UNIX_INFO));
> >> pSMB->SearchFlags = cpu_to_le16(search_flags);
> >> pSMB->InformationLevel = cpu_to_le16(psrch_inf->info_level);
> >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> >> index 8e8bb49..a0ed97d 100644
> >> --- a/fs/cifs/inode.c
> >> +++ b/fs/cifs/inode.c
> >> @@ -600,17 +600,40 @@ cgfi_exit:
> >> return rc;
> >> }
> >>
> >> +static void
> >> +fidinfo2fainfo(SEARCH_ID_FULL_DIR_INFO *fidinfo, FILE_ALL_INFO *fainfo)
> >> +{
> >> + memcpy(&fainfo->CreationTime, &fidinfo->CreationTime, sizeof(__le64));
> >> + memcpy(&fainfo->LastAccessTime, &fidinfo->LastAccessTime,
> >> + sizeof(__le64));
> >> + memcpy(&fainfo->LastWriteTime, &fidinfo->LastWriteTime,
> >> + sizeof(__le64));
> >> + memcpy(&fainfo->ChangeTime, &fidinfo->ChangeTime,
> >> + sizeof(__le64));
> >> + memcpy(&fainfo->EndOfFile, &fidinfo->EndOfFile,
> >> + sizeof(__le64));
> >> + memcpy(&fainfo->AllocationSize, &fidinfo->AllocationSize,
> >> + sizeof(__le64));
> >> + memcpy(&fainfo->Attributes, &fidinfo->ExtFileAttributes,
> >> + sizeof(__le32));
> >> +
> >> + return;
> >> +}
> >> +
> >
> >
> > Yuck. So you're going to take the SEARCH_ID_FULL_DIR_INFO and convert
> > that to FILE_ALL_INFO, just so it can then be converted to a
> > cifs_fattr? Why not shortcut this step, and turn it into a fattr
> > directly. I think we already have some routines to do that.
>
> I will look into code to use any existing one, if not write a new similar one
> that converts search_id_full_dir_info to fattr.
>
There are such functions in the readdir code, but you'll probably have
to handle the cf_uniqueid separately.
> >
> >> int cifs_get_inode_info(struct inode **pinode,
> >> const unsigned char *full_path, FILE_ALL_INFO *pfindData,
> >> struct super_block *sb, int xid, const __u16 *pfid)
> >> {
> >> - int rc = 0, tmprc;
> >> + __u16 fid, srchflgs;
> >> + int rc = 0, rc2 = -EINVAL, tmprc;
> >> struct cifs_tcon *pTcon;
> >> struct tcon_link *tlink;
> >> struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
> >> char *buf = NULL;
> >> bool adjustTZ = false;
> >> struct cifs_fattr fattr;
> >> + struct cifs_search_info *psrch_inf = NULL;
> >> + SEARCH_ID_FULL_DIR_INFO *ffdata = NULL;
> >>
> >> tlink = cifs_sb_tlink(cifs_sb);
> >> if (IS_ERR(tlink))
> >> @@ -640,6 +663,38 @@ int cifs_get_inode_info(struct inode **pinode,
> >> 0 /* not legacy */,
> >> cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
> >> CIFS_MOUNT_MAP_SPECIAL_CHR);
> >> +
> >> + /*
> >> + * This findfirst call is meant for a lookup operation
> >> + * that would otherwise fail since query path info has no
> >> + * means to indicate backup intent.
> >> + */
> >> + if (rc == -EACCES && *pinode == NULL && backup_cred(cifs_sb)) {
> >> + psrch_inf = kzalloc(sizeof(struct cifs_search_info),
> >> + GFP_KERNEL);
> >> + if (psrch_inf == NULL) {
> >> + rc = -ENOMEM;
> >> + goto cgii_exit;
> >> + }
> >> + psrch_inf->endOfSearch = false;
> >> + psrch_inf->info_level = SMB_FIND_FILE_ID_FULL_DIR_INFO;
> >> +
> >> + srchflgs = CIFS_SEARCH_CLOSE_ALWAYS |
> >> + CIFS_SEARCH_CLOSE_AT_END |
> >> + CIFS_SEARCH_BACKUP_SEARCH;
> >> +
> >> + rc2 = CIFSFindFirst(xid, pTcon, full_path,
> >> + cifs_sb->local_nls, &fid, srchflgs, psrch_inf,
> >> + cifs_sb->mnt_cifs_flags &
> >> + CIFS_MOUNT_MAP_SPECIAL_CHR, 0);
> >> + if (!rc2) {
> >> + ffdata = (SEARCH_ID_FULL_DIR_INFO *)
> >> + psrch_inf->srch_entries_start;
> >> + fidinfo2fainfo(ffdata, pfindData);
> >> + }
> >> + rc = rc2;
> >> + }
> >> +
> >> /* BB optimize code so we do not make the above call
> >> when server claims no NT SMB support and the above call
> >> failed at least once - set flag in tcon or mount */
> >> @@ -683,7 +738,11 @@ int cifs_get_inode_info(struct inode **pinode,
> >> if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) {
> >> int rc1 = 0;
> >>
> >> - rc1 = CIFSGetSrvInodeNumber(xid, pTcon,
> >> + if (!rc2)
> >> + fattr.cf_uniqueid =
> >> + le64_to_cpu(ffdata->UniqueId);
> >
> > Won't ffdata only be non-NULL if you fall into the new code block
> > above? That looks like it could oops.
>
> rc2 is set as -EINVAL. If we do not fall into the new code, rc2 is not 0
> i.e. !rc2 is false. If we fall into new code and findfirst call is successful,
> rc2 is 0 and !rc2 is true but if findfirst call fails, rc2 is not 0 and
> so !rc2 is false. So I do not think we will oops because ffdata is NULL.
>
Ick ok. That's very subtle. It would be nice to clean up this function
some so that there are not so many subtleties. It's probably time to
break it up into multiple functions.
> >
> >> + else
> >> + rc1 = CIFSGetSrvInodeNumber(xid, pTcon,
> >> full_path, &fattr.cf_uniqueid,
> >> cifs_sb->local_nls,
> >> cifs_sb->mnt_cifs_flags &
> >> @@ -741,6 +800,10 @@ int cifs_get_inode_info(struct inode **pinode,
> >> }
> >>
> >> cgii_exit:
> >> + if (!rc2) {
> >> + cifs_buf_release(psrch_inf->ntwrk_buf_start);
> >> + kfree(psrch_inf);
> >> + }
> >> kfree(buf);
> >> cifs_put_tlink(tlink);
> >> return rc;
> >
> >
--
Jeff Layton <jlayton@xxxxxxxxx>
--
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
