On Thu, Aug 2, 2012 at 9:37 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> 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.
I will spend some time on using dirsep and making use of dirsep
more (self) explanatory.
What I see is, findfirst does not need to be employed because
access is not denied when query'ing attributes of a directory
i.e. trans2/query path info/infolevel always succeeds in case of a
directory.
So when we are (for now) using findfirst as a substitute when
query path info is denied access on a file, ATTR_DIRECTORY
is never needed and until this point, I was using dirsep to distinguish
that in common findfirst code.
>
> 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
