[PATCH] Fix samba bug #8914

[Gregor Beck]

Hi,

NetApp sends brocken packages in response to trans2_query_path_info with info
level smb_file_all_info as reported at:
https://bugzilla.samba.org/show_bug.cgi?id=8914

The attached patch for cifs 2.01 works around the problem by retrying the
operation with info levels basic and standard which yield the same
information.

Thanks
Gregor

[Jeff Layton]

On Thu, 21 Nov 2013 12:51:43 +0100

Post by Gregor Beck
Hi,
NetApp sends brocken packages in response to trans2_query_path_info with info
https://bugzilla.samba.org/show_bug.cgi?id=8914
The attached patch for cifs 2.01 works around the problem by retrying the
operation with info levels basic and standard which yield the same
information.
Thanks
Gregor

Could you repost these in the standard way?

One patch per email

Also, inlined in the email instead of as attachments, but we can work
with it if you need to send them as attachments. git-send-email is a
good tool for this, btw...

[Gregor Beck]

Hi,

my first use of git-send-email and sendmail ;-)

Gregor

[Gregor Beck]

---
cifspdu.h | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/cifspdu.h b/cifspdu.h
index 11ca24a..89aa1f2 100644
--- a/cifspdu.h
+++ b/cifspdu.h
@@ -2233,6 +2233,16 @@ typedef struct { /* data block encoding of response to level 263 QPathInfo */
char FileName[1];
} __attribute__((packed)) FILE_ALL_INFO; /* level 0x107 QPathInfo */

+typedef struct {
+ __le64 AllocationSize;
+ __le64 EndOfFile; /* size ie offset to first free byte in file */
+ __le32 NumberOfLinks; /* hard links */
+ __u8 DeletePending;
+ __u8 Directory;
+ __u16 Pad;
+} __attribute__((packed)) FILE_STANDARD_INFO; /* level 0x102 QPathInfo */
+
+
/* defines for enumerating possible values of the Unix type field below */
#define UNIX_FILE 0
#define UNIX_DIR 1

[Jeff Layton]

On Fri, 22 Nov 2013 11:26:50 +0100

Post by Gregor Beck
---
cifspdu.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/cifspdu.h b/cifspdu.h
index 11ca24a..89aa1f2 100644
--- a/cifspdu.h
+++ b/cifspdu.h
@@ -2233,6 +2233,16 @@ typedef struct { /* data block encoding of response to level 263 QPathInfo */
char FileName[1];
} __attribute__((packed)) FILE_ALL_INFO; /* level 0x107 QPathInfo */
+typedef struct {
+ __le64 AllocationSize;
+ __le64 EndOfFile; /* size ie offset to first free byte in file */
+ __le32 NumberOfLinks; /* hard links */
+ __u8 DeletePending;
+ __u8 Directory;
+ __u16 Pad;
+} __attribute__((packed)) FILE_STANDARD_INFO; /* level 0x102 QPathInfo */
+
+
/* defines for enumerating possible values of the Unix type field below */
#define UNIX_FILE 0
#define UNIX_DIR 1

Looks correct...

Acked-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+***@public.gmane.org>

[Gregor Beck]

---
cifssmb.c | 59 ++++++++++++++++++++++++++++++++---------------------------
1 file changed, 32 insertions(+), 27 deletions(-)

diff --git a/cifssmb.c b/cifssmb.c
index a89c4cb..7b055fe 100644
--- a/cifssmb.c
+++ b/cifssmb.c
@@ -3958,13 +3958,13 @@ QFileInfoRetry:
return rc;
}

-int
-CIFSSMBQPathInfo(const unsigned int xid, struct cifs_tcon *tcon,
- const char *search_name, FILE_ALL_INFO *data,
- int legacy /* old style infolevel */,
- const struct nls_table *nls_codepage, int remap)
+static int
+CIFSSMBQPathInfoImpl(const unsigned int xid, struct cifs_tcon *tcon,
+ const char *search_name,
+ void *data, int size,
+ __u16 level, __u16 bcc,
+ const struct nls_table *nls_codepage, int remap)
{
- /* level 263 SMB_QUERY_FILE_ALL_INFO */
TRANSACTION2_QPI_REQ *pSMB = NULL;
TRANSACTION2_QPI_RSP *pSMBr = NULL;
int rc = 0;
@@ -3972,7 +3972,7 @@ CIFSSMBQPathInfo(const unsigned int xid, struct cifs_tcon *tcon,
int name_len;
__u16 params, byte_count;

- /* cifs_dbg(FYI, "In QPathInfo path %s\n", search_name); */
+ cifs_dbg(FYI, "In QPathInfo level %u path %s", level, search_name);
QPathInfoRetry:
rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
(void **) &pSMBr);
@@ -4011,10 +4011,7 @@ QPathInfoRetry:
byte_count = params + 1 /* pad */ ;
pSMB->TotalParameterCount = cpu_to_le16(params);
pSMB->ParameterCount = pSMB->TotalParameterCount;
- if (legacy)
- pSMB->InformationLevel = cpu_to_le16(SMB_INFO_STANDARD);
- else
- pSMB->InformationLevel = cpu_to_le16(SMB_QUERY_FILE_ALL_INFO);
+ pSMB->InformationLevel = level;
pSMB->Reserved4 = 0;
inc_rfc1001_len(pSMB, byte_count);
pSMB->ByteCount = cpu_to_le16(byte_count);
@@ -4028,25 +4025,10 @@ QPathInfoRetry:

if (rc) /* BB add auto retry on EOPNOTSUPP? */
rc = -EIO;
- else if (!legacy && get_bcc(&pSMBr->hdr) < 40)
+ else if (get_bcc(&pSMBr->hdr) < bcc)
rc = -EIO; /* bad smb */
- else if (legacy && get_bcc(&pSMBr->hdr) < 24)
- rc = -EIO; /* 24 or 26 expected but we do not read
- last field */
else if (data) {
- int size;
__u16 data_offset = le16_to_cpu(pSMBr->t2.DataOffset);
-
- /*
- * On legacy responses we do not read the last field,
- * EAsize, fortunately since it varies by subdialect and
- * also note it differs on Set vs Get, ie two bytes or 4
- * bytes depending but we don't care here.
- */
- if (legacy)
- size = sizeof(FILE_INFO_STANDARD);
- else
- size = sizeof(FILE_ALL_INFO);
memcpy((char *) data, (char *) &pSMBr->hdr.Protocol +
data_offset, size);
} else
@@ -4060,6 +4042,29 @@ QPathInfoRetry:
}

int
+CIFSSMBQPathInfo(const unsigned int xid, struct cifs_tcon *tcon,
+ const char *search_name, FILE_ALL_INFO *data,
+ int legacy /* old style infolevel */,
+ const struct nls_table *nls_codepage, int remap)
+{
+ if (legacy) {
+ /* 24 or 26 expected but on legacy responses we do not read the
+ last field, EAsize, fortunately since it varies by subdialect
+ and also note it differs on Set vs. Get, ie two bytes or 4
+ bytes depending but we don't care here */
+ return CIFSSMBQPathInfoImpl(xid, tcon, search_name,
+ data, sizeof(FILE_INFO_STANDARD),
+ SMB_INFO_STANDARD, 24,
+ nls_codepage, remap);
+ } else {
+ return CIFSSMBQPathInfoImpl(xid, tcon, search_name,
+ data, sizeof(FILE_ALL_INFO),
+ SMB_QUERY_FILE_ALL_INFO, 40,
+ nls_codepage, remap);
+ }
+}
+
+int
CIFSSMBUnixQFileInfo(const unsigned int xid, struct cifs_tcon *tcon,
u16 netfid, FILE_UNIX_BASIC_INFO *pFindData)
{

[Jeff Layton]

On Fri, 22 Nov 2013 11:26:51 +0100

Post by Gregor Beck
---
cifssmb.c | 59 ++++++++++++++++++++++++++++++++---------------------------
1 file changed, 32 insertions(+), 27 deletions(-)
diff --git a/cifssmb.c b/cifssmb.c
index a89c4cb..7b055fe 100644
--- a/cifssmb.c
+++ b/cifssmb.c
return rc;
}
-int
-CIFSSMBQPathInfo(const unsigned int xid, struct cifs_tcon *tcon,
- const char *search_name, FILE_ALL_INFO *data,
- int legacy /* old style infolevel */,
- const struct nls_table *nls_codepage, int remap)
+static int
+CIFSSMBQPathInfoImpl(const unsigned int xid, struct cifs_tcon *tcon,
+ const char *search_name,
+ void *data, int size,
+ __u16 level, __u16 bcc,
+ const struct nls_table *nls_codepage, int remap)
{
- /* level 263 SMB_QUERY_FILE_ALL_INFO */
TRANSACTION2_QPI_REQ *pSMB = NULL;
TRANSACTION2_QPI_RSP *pSMBr = NULL;
int rc = 0;
@@ -3972,7 +3972,7 @@ CIFSSMBQPathInfo(const unsigned int xid, struct cifs_tcon *tcon,
int name_len;
__u16 params, byte_count;
- /* cifs_dbg(FYI, "In QPathInfo path %s\n", search_name); */
+ cifs_dbg(FYI, "In QPathInfo level %u path %s", level, search_name);
rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
(void **) &pSMBr);
byte_count = params + 1 /* pad */ ;
pSMB->TotalParameterCount = cpu_to_le16(params);
pSMB->ParameterCount = pSMB->TotalParameterCount;
- if (legacy)
- pSMB->InformationLevel = cpu_to_le16(SMB_INFO_STANDARD);
- else
- pSMB->InformationLevel = cpu_to_le16(SMB_QUERY_FILE_ALL_INFO);
+ pSMB->InformationLevel = level;

should be "cpu_to_le16(level)"

Post by Gregor Beck
pSMB->Reserved4 = 0;
inc_rfc1001_len(pSMB, byte_count);
pSMB->ByteCount = cpu_to_le16(byte_count);
if (rc) /* BB add auto retry on EOPNOTSUPP? */
rc = -EIO;
- else if (!legacy && get_bcc(&pSMBr->hdr) < 40)
+ else if (get_bcc(&pSMBr->hdr) < bcc)
rc = -EIO; /* bad smb */
- else if (legacy && get_bcc(&pSMBr->hdr) < 24)
- rc = -EIO; /* 24 or 26 expected but we do not read
- last field */
else if (data) {
- int size;
__u16 data_offset = le16_to_cpu(pSMBr->t2.DataOffset);
-
- /*
- * On legacy responses we do not read the last field,
- * EAsize, fortunately since it varies by subdialect and
- * also note it differs on Set vs Get, ie two bytes or 4
- * bytes depending but we don't care here.
- */
- if (legacy)
- size = sizeof(FILE_INFO_STANDARD);
- else
- size = sizeof(FILE_ALL_INFO);
memcpy((char *) data, (char *) &pSMBr->hdr.Protocol +
data_offset, size);
} else
}
int
+CIFSSMBQPathInfo(const unsigned int xid, struct cifs_tcon *tcon,
+ const char *search_name, FILE_ALL_INFO *data,
+ int legacy /* old style infolevel */,
+ const struct nls_table *nls_codepage, int remap)
+{
+ if (legacy) {
+ /* 24 or 26 expected but on legacy responses we do not read the
+ last field, EAsize, fortunately since it varies by subdialect
+ and also note it differs on Set vs. Get, ie two bytes or 4
+ bytes depending but we don't care here */

nit: The above comment should be in kernel standard format (like the one is
that you removed).

Post by Gregor Beck
+ return CIFSSMBQPathInfoImpl(xid, tcon, search_name,
+ data, sizeof(FILE_INFO_STANDARD),
+ SMB_INFO_STANDARD, 24,
+ nls_codepage, remap);
+ } else {
+ return CIFSSMBQPathInfoImpl(xid, tcon, search_name,
+ data, sizeof(FILE_ALL_INFO),
+ SMB_QUERY_FILE_ALL_INFO, 40,
+ nls_codepage, remap);

Avoid using "naked" numbers like you are with the bcc values above. At
the very least if you do use them, please add comments to explain why
they are what they are.

Yes, it's true that the existing code does that but it's nasty, and the
time to clean it up is while you're in there.

Post by Gregor Beck
+ }
+}
+
+int
CIFSSMBUnixQFileInfo(const unsigned int xid, struct cifs_tcon *tcon,
u16 netfid, FILE_UNIX_BASIC_INFO *pFindData)
{

Other than the above, this looks like a reasonable cleanup.

[Gregor Beck]

---
cifsproto.h | 3 +++
cifssmb.c | 11 +++++++++++
2 files changed, 14 insertions(+)

diff --git a/cifsproto.h b/cifsproto.h
index b29a012..ea25d0d 100644
--- a/cifsproto.h
+++ b/cifsproto.h
@@ -240,6 +240,9 @@ extern int CIFSSMBQPathInfo(const unsigned int xid, struct cifs_tcon *tcon,
const char *search_Name, FILE_ALL_INFO *data,
int legacy /* whether to use old info level */,
const struct nls_table *nls_codepage, int remap);
+extern int CIFSSMBQPathInfoBasic(const unsigned int xid, struct cifs_tcon *tcon,
+ const char *search_name, FILE_BASIC_INFO *data,
+ const struct nls_table *nls_codepage, int remap);
extern int SMBQueryInformation(const unsigned int xid, struct cifs_tcon *tcon,
const char *search_name, FILE_ALL_INFO *data,
const struct nls_table *nls_codepage, int remap);
diff --git a/cifssmb.c b/cifssmb.c
index 7b055fe..d8189b5 100644
--- a/cifssmb.c
+++ b/cifssmb.c
@@ -4065,6 +4065,17 @@ CIFSSMBQPathInfo(const unsigned int xid, struct cifs_tcon *tcon,
}

int
+CIFSSMBQPathInfoBasic(const unsigned int xid, struct cifs_tcon *tcon,
+ const char *search_name, FILE_BASIC_INFO *data,
+ const struct nls_table *nls_codepage, int remap)
+{
+ return CIFSSMBQPathInfoImpl(xid, tcon, search_name,
+ data, sizeof(FILE_BASIC_INFO),
+ SMB_QUERY_FILE_BASIC_INFO, 40 /*???*/,
+ nls_codepage, remap);
+}
+
+int
CIFSSMBUnixQFileInfo(const unsigned int xid, struct cifs_tcon *tcon,
u16 netfid, FILE_UNIX_BASIC_INFO *pFindData)
{

[Gregor Beck]

---
cifsproto.h | 3 +++
cifssmb.c | 11 +++++++++++
2 files changed, 14 insertions(+)

diff --git a/cifsproto.h b/cifsproto.h
index ea25d0d..d015336 100644
--- a/cifsproto.h
+++ b/cifsproto.h
@@ -243,6 +243,9 @@ extern int CIFSSMBQPathInfo(const unsigned int xid, struct cifs_tcon *tcon,
extern int CIFSSMBQPathInfoBasic(const unsigned int xid, struct cifs_tcon *tcon,
const char *search_name, FILE_BASIC_INFO *data,
const struct nls_table *nls_codepage, int remap);
+extern int CIFSSMBQPathInfoStandard(const unsigned int xid, struct cifs_tcon *tcon,
+ const char *search_name, FILE_STANDARD_INFO *data,
+ const struct nls_table *nls_codepage, int remap);
extern int SMBQueryInformation(const unsigned int xid, struct cifs_tcon *tcon,
const char *search_name, FILE_ALL_INFO *data,
const struct nls_table *nls_codepage, int remap);
diff --git a/cifssmb.c b/cifssmb.c
index d8189b5..38be854 100644
--- a/cifssmb.c
+++ b/cifssmb.c
@@ -4076,6 +4076,17 @@ CIFSSMBQPathInfoBasic(const unsigned int xid, struct cifs_tcon *tcon,
}

int
+CIFSSMBQPathInfoStandard(const unsigned int xid, struct cifs_tcon *tcon,
+ const char *search_name, FILE_STANDARD_INFO *data,
+ const struct nls_table *nls_codepage, int remap)
+{
+ return CIFSSMBQPathInfoImpl(xid, tcon, search_name,
+ data, sizeof(FILE_STANDARD_INFO),
+ SMB_QUERY_FILE_STANDARD_INFO, 24 /*???*/,
+ nls_codepage, remap);
+}
+
+int
CIFSSMBUnixQFileInfo(const unsigned int xid, struct cifs_tcon *tcon,
u16 netfid, FILE_UNIX_BASIC_INFO *pFindData)
{

[Gregor Beck]

---
smb1ops.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/smb1ops.c b/smb1ops.c
index 6094397..635b1e5 100644
--- a/smb1ops.c
+++ b/smb1ops.c
@@ -513,20 +513,23 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
{
int rc;
FILE_ALL_INFO *file_info;
+ const int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;

file_info = kmalloc(sizeof(FILE_ALL_INFO), GFP_KERNEL);
if (file_info == NULL)
return -ENOMEM;

rc = CIFSSMBQPathInfo(xid, tcon, full_path, file_info,
- 0 /* not legacy */, cifs_sb->local_nls,
- cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
+ 0 /* not legacy */, cifs_sb->local_nls, remap);
+ if (rc == -EIO) {
+ rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
+ (FILE_BASIC_INFO*)file_info,
+ cifs_sb->local_nls, remap);
+ }

if (rc == -EOPNOTSUPP || rc == -EINVAL)
rc = SMBQueryInformation(xid, tcon, full_path, file_info,
- cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
+ cifs_sb->local_nls, remap);
kfree(file_info);
return rc;
}

[Jeff Layton]

On Fri, 22 Nov 2013 11:26:54 +0100

Post by Gregor Beck
---
smb1ops.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/smb1ops.c b/smb1ops.c
index 6094397..635b1e5 100644
--- a/smb1ops.c
+++ b/smb1ops.c
@@ -513,20 +513,23 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
{
int rc;
FILE_ALL_INFO *file_info;
+ const int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;

So many of the arguments to these functions always come from the
cifs_sb. Would it be reasonable to simply pass a cifs_sb pointer down
to these functions and cut out some of the argument bloat?

Post by Gregor Beck
file_info = kmalloc(sizeof(FILE_ALL_INFO), GFP_KERNEL);
if (file_info == NULL)
return -ENOMEM;
rc = CIFSSMBQPathInfo(xid, tcon, full_path, file_info,
- 0 /* not legacy */, cifs_sb->local_nls,
- cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
+ 0 /* not legacy */, cifs_sb->local_nls, remap);
+ if (rc == -EIO) {
+ rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
+ (FILE_BASIC_INFO*)file_info,
+ cifs_sb->local_nls, remap);
+ }
if (rc == -EOPNOTSUPP || rc == -EINVAL)
rc = SMBQueryInformation(xid, tcon, full_path, file_info,
- cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
+ cifs_sb->local_nls, remap);
kfree(file_info);
return rc;
}

-EIO can happen for a lot of different reasons, but there's little harm
in retrying with a more basic call in this function. In fact, it might
make more sense to just always use CIFSSMBQPathInfoBasic here since we
don't actually do anything with the returned info.

[Christoph Hellwig]

Please provide a helper that encapsulates the calls for the different
info levels, and then formats them into a in-kernel sanitized structure
instead of casting around the on the wire structures, similar to how
the directory code translates that various info levels in a cifs_dirent
structure.

[Jeff Layton]

On Fri, 22 Nov 2013 06:35:46 -0800

Post by Christoph Hellwig
Please provide a helper that encapsulates the calls for the different
info levels, and then formats them into a in-kernel sanitized structure
instead of casting around the on the wire structures, similar to how
the directory code translates that various info levels in a cifs_dirent
structure.

Agreed. What we really need to do is to have these functions pass
around the container we already have for this: a struct cifs_fattr
rather than FILE_ALL_INFO. It's really an API abortion that even the
SMB2 calls have to pass around an SMB1 on-the-wire data structure.

Gregor, it would be best to do a patchset to fix that mess first and
then layer these changes on top of it.

[Gregor Beck]

---
smb1ops.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/smb1ops.c b/smb1ops.c
index 635b1e5..a12e370 100644
--- a/smb1ops.c
+++ b/smb1ops.c
@@ -540,11 +540,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
FILE_ALL_INFO *data, bool *adjustTZ)
{
int rc;
+ const int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;

/* could do find first instead but this returns more info */
rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */,
- cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
+ cifs_sb->local_nls, remap);
+
+ if (rc == -EIO) {
+ rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
+ (FILE_BASIC_INFO*)data,
+ cifs_sb->local_nls, remap);
+ if (!rc) {
+ rc = CIFSSMBQPathInfoStandard(xid, tcon, full_path,
+ (void*)data + sizeof(FILE_BASIC_INFO),
+ cifs_sb->local_nls, remap);
+ }
+ }
+
/*
* 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
@@ -552,9 +564,7 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
*/
if ((rc == -EOPNOTSUPP) || (rc == -EINVAL)) {
rc = SMBQueryInformation(xid, tcon, full_path, data,
- cifs_sb->local_nls,
- cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
+ cifs_sb->local_nls, remap);
*adjustTZ = true;
}
return rc;

[Jeff Layton]

On Fri, 22 Nov 2013 11:26:55 +0100
Gregor Beck <gbeck-3ekOc4rQMZmzQB+***@public.gmane.org> wrote:


Some explanation of the rationale for this might be nice.

Post by Gregor Beck
---
smb1ops.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/smb1ops.c b/smb1ops.c
index 635b1e5..a12e370 100644
--- a/smb1ops.c
+++ b/smb1ops.c
@@ -540,11 +540,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
FILE_ALL_INFO *data, bool *adjustTZ)
{
int rc;
+ const int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;
/* could do find first instead but this returns more info */
rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */,
- cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
+ cifs_sb->local_nls, remap);
+
+ if (rc == -EIO) {
+ rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
+ (FILE_BASIC_INFO*)data,
+ cifs_sb->local_nls, remap);
+ if (!rc) {
+ rc = CIFSSMBQPathInfoStandard(xid, tcon, full_path,
+ (void*)data + sizeof(FILE_BASIC_INFO),
+ cifs_sb->local_nls, remap);
+ }
+ }
+

I don't get the above. Why do the Basic call at all if you're just
going to overwrite the returned info with the standard one when it
succeeds?

Post by Gregor Beck
/*
* 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
@@ -552,9 +564,7 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
*/
if ((rc == -EOPNOTSUPP) || (rc == -EINVAL)) {
rc = SMBQueryInformation(xid, tcon, full_path, data,
- cifs_sb->local_nls,
- cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
+ cifs_sb->local_nls, remap);
*adjustTZ = true;
}
return rc;

[Gregor Beck]

---
cifssmb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cifssmb.c b/cifssmb.c
index 38be854..5d10afc 100644
--- a/cifssmb.c
+++ b/cifssmb.c
@@ -3935,7 +3935,7 @@ QFileInfoRetry:
rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
(struct smb_hdr *) pSMBr, &bytes_returned, 0);
if (rc) {
- cifs_dbg(FYI, "Send error in QPathInfo = %d\n", rc);
+ cifs_dbg(FYI, "Send error in QFileInfo = %d", rc);
} else { /* decode response */
rc = validate_t2((struct smb_t2_rsp *)pSMBr);

@@ -4131,7 +4131,7 @@ UnixQFileInfoRetry:
rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
(struct smb_hdr *) pSMBr, &bytes_returned, 0);
if (rc) {
- cifs_dbg(FYI, "Send error in QPathInfo = %d\n", rc);
+ cifs_dbg(FYI, "Send error in UnixQFileInfo = %d", rc);
} else { /* decode response */
rc = validate_t2((struct smb_t2_rsp *)pSMBr);

@@ -4215,7 +4215,7 @@ UnixQPathInfoRetry:
rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
(struct smb_hdr *) pSMBr, &bytes_returned, 0);
if (rc) {
- cifs_dbg(FYI, "Send error in QPathInfo = %d\n", rc);
+ cifs_dbg(FYI, "Send error in UnixQPathInfo = %d", rc);
} else { /* decode response */
rc = validate_t2((struct smb_t2_rsp *)pSMBr);

[Jeff Layton]

On Fri, 22 Nov 2013 11:26:56 +0100

Post by Gregor Beck
---
cifssmb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/cifssmb.c b/cifssmb.c
index 38be854..5d10afc 100644
--- a/cifssmb.c
+++ b/cifssmb.c
rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
(struct smb_hdr *) pSMBr, &bytes_returned, 0);
if (rc) {
- cifs_dbg(FYI, "Send error in QPathInfo = %d\n", rc);
+ cifs_dbg(FYI, "Send error in QFileInfo = %d", rc);
} else { /* decode response */
rc = validate_t2((struct smb_t2_rsp *)pSMBr);
rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
(struct smb_hdr *) pSMBr, &bytes_returned, 0);
if (rc) {
- cifs_dbg(FYI, "Send error in QPathInfo = %d\n", rc);
+ cifs_dbg(FYI, "Send error in UnixQFileInfo = %d", rc);
} else { /* decode response */
rc = validate_t2((struct smb_t2_rsp *)pSMBr);
rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
(struct smb_hdr *) pSMBr, &bytes_returned, 0);
if (rc) {
- cifs_dbg(FYI, "Send error in QPathInfo = %d\n", rc);
+ cifs_dbg(FYI, "Send error in UnixQPathInfo = %d", rc);
} else { /* decode response */
rc = validate_t2((struct smb_t2_rsp *)pSMBr);

Nice fix. I'd go ahead and ask Steve to merge this independently of the others...

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+***@public.gmane.org>

[Gregor Beck]

---
smb1ops.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/smb1ops.c b/smb1ops.c
index a12e370..11daa28 100644
--- a/smb1ops.c
+++ b/smb1ops.c
@@ -522,6 +522,7 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
rc = CIFSSMBQPathInfo(xid, tcon, full_path, file_info,
0 /* not legacy */, cifs_sb->local_nls, remap);
if (rc == -EIO) {
+ cifs_dbg(FYI, "is_path_accessible: FALLBACK");
rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
(FILE_BASIC_INFO*)file_info,
cifs_sb->local_nls, remap);
@@ -547,6 +548,7 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
cifs_sb->local_nls, remap);

if (rc == -EIO) {
+ cifs_dbg(FYI, "cifs_query_path_info: FALLBACK");
rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
(FILE_BASIC_INFO*)data,
cifs_sb->local_nls, remap);

[Jeff Layton]

On Fri, 22 Nov 2013 11:26:57 +0100
Gregor Beck <gbeck-3ekOc4rQMZmzQB+***@public.gmane.org> wrote:

The subject on this patch needs more elaboration. "cFYI" doesn't tell
me much when I'm looking at this patch in a year or two.

Post by Gregor Beck
---
smb1ops.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/smb1ops.c b/smb1ops.c
index a12e370..11daa28 100644
--- a/smb1ops.c
+++ b/smb1ops.c
@@ -522,6 +522,7 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
rc = CIFSSMBQPathInfo(xid, tcon, full_path, file_info,
0 /* not legacy */, cifs_sb->local_nls, remap);
if (rc == -EIO) {
+ cifs_dbg(FYI, "is_path_accessible: FALLBACK");
rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
(FILE_BASIC_INFO*)file_info,
cifs_sb->local_nls, remap);
@@ -547,6 +548,7 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
cifs_sb->local_nls, remap);
if (rc == -EIO) {
+ cifs_dbg(FYI, "cifs_query_path_info: FALLBACK");
rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
(FILE_BASIC_INFO*)data,
cifs_sb->local_nls, remap);

[Gregor Beck]

---
cifsglob.h | 1 +
smb1ops.c | 26 ++++++++++++++++++--------
2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/cifsglob.h b/cifsglob.h
index 52ca861..d3adde7 100644
--- a/cifsglob.h
+++ b/cifsglob.h
@@ -818,6 +818,7 @@ struct cifs_tcon {
bool local_lease:1; /* check leases (only) on local system not remote */
bool broken_posix_open; /* e.g. Samba server versions < 3.3.2, 3.2.9 */
bool need_reconnect:1; /* connection reset, tid now invalid */
+ bool broken_qpath_info:1;
#ifdef CONFIG_CIFS_SMB2
bool print:1; /* set if connection to printer share */
bool bad_network_name:1; /* set if ret status STATUS_BAD_NETWORK_NAME */
diff --git a/smb1ops.c b/smb1ops.c
index 11daa28..e3798a4 100644
--- a/smb1ops.c
+++ b/smb1ops.c
@@ -511,7 +511,7 @@ static int
cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
struct cifs_sb_info *cifs_sb, const char *full_path)
{
- int rc;
+ int rc= -EIO;
FILE_ALL_INFO *file_info;
const int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;

@@ -519,13 +519,18 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
if (file_info == NULL)
return -ENOMEM;

- rc = CIFSSMBQPathInfo(xid, tcon, full_path, file_info,
- 0 /* not legacy */, cifs_sb->local_nls, remap);
+ if (!tcon->broken_qpath_info) {
+ rc = CIFSSMBQPathInfo(xid, tcon, full_path, file_info,
+ 0, cifs_sb->local_nls, remap);
+ }
if (rc == -EIO) {
- cifs_dbg(FYI, "is_path_accessible: FALLBACK");
rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
(FILE_BASIC_INFO*)file_info,
cifs_sb->local_nls, remap);
+ cifs_dbg(FYI, "is_path_accessible: FALLBACK returns %d", rc);
+ if (!rc) {
+ tcon->broken_qpath_info = true;
+ }
}

if (rc == -EOPNOTSUPP || rc == -EINVAL)
@@ -540,15 +545,16 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
struct cifs_sb_info *cifs_sb, const char *full_path,
FILE_ALL_INFO *data, bool *adjustTZ)
{
- int rc;
+ int rc = -EIO;
const int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;

/* could do find first instead but this returns more info */
- rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */,
- cifs_sb->local_nls, remap);
+ if (!tcon->broken_qpath_info) {
+ rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0,
+ cifs_sb->local_nls, remap);
+ }

if (rc == -EIO) {
- cifs_dbg(FYI, "cifs_query_path_info: FALLBACK");
rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
(FILE_BASIC_INFO*)data,
cifs_sb->local_nls, remap);
@@ -557,6 +563,10 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
(void*)data + sizeof(FILE_BASIC_INFO),
cifs_sb->local_nls, remap);
}
+ cifs_dbg(FYI, "cifs_query_path_info: FALLBACK returns %d", rc);
+ if (!rc) {
+ tcon->broken_qpath_info = true;
+ }
}

/*

[Jeff Layton]

On Fri, 22 Nov 2013 11:26:58 +0100

Post by Gregor Beck
---
cifsglob.h | 1 +
smb1ops.c | 26 ++++++++++++++++++--------
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/cifsglob.h b/cifsglob.h
index 52ca861..d3adde7 100644
--- a/cifsglob.h
+++ b/cifsglob.h
@@ -818,6 +818,7 @@ struct cifs_tcon {
bool local_lease:1; /* check leases (only) on local system not remote */
bool broken_posix_open; /* e.g. Samba server versions < 3.3.2, 3.2.9 */
bool need_reconnect:1; /* connection reset, tid now invalid */
+ bool broken_qpath_info:1;
#ifdef CONFIG_CIFS_SMB2
bool print:1; /* set if connection to printer share */
bool bad_network_name:1; /* set if ret status STATUS_BAD_NETWORK_NAME */
diff --git a/smb1ops.c b/smb1ops.c
index 11daa28..e3798a4 100644
--- a/smb1ops.c
+++ b/smb1ops.c
@@ -511,7 +511,7 @@ static int
cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
struct cifs_sb_info *cifs_sb, const char *full_path)
{
- int rc;
+ int rc= -EIO;
FILE_ALL_INFO *file_info;
const int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;
@@ -519,13 +519,18 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
if (file_info == NULL)
return -ENOMEM;
- rc = CIFSSMBQPathInfo(xid, tcon, full_path, file_info,
- 0 /* not legacy */, cifs_sb->local_nls, remap);
+ if (!tcon->broken_qpath_info) {
+ rc = CIFSSMBQPathInfo(xid, tcon, full_path, file_info,
+ 0, cifs_sb->local_nls, remap);
+ }
if (rc == -EIO) {
- cifs_dbg(FYI, "is_path_accessible: FALLBACK");
rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
(FILE_BASIC_INFO*)file_info,
cifs_sb->local_nls, remap);
+ cifs_dbg(FYI, "is_path_accessible: FALLBACK returns %d", rc);
+ if (!rc) {
+ tcon->broken_qpath_info = true;
+ }
}
if (rc == -EOPNOTSUPP || rc == -EINVAL)
@@ -540,15 +545,16 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
struct cifs_sb_info *cifs_sb, const char *full_path,
FILE_ALL_INFO *data, bool *adjustTZ)
{
- int rc;
+ int rc = -EIO;
const int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;
/* could do find first instead but this returns more info */
- rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */,
- cifs_sb->local_nls, remap);
+ if (!tcon->broken_qpath_info) {
+ rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0,
+ cifs_sb->local_nls, remap);
+ }
if (rc == -EIO) {
- cifs_dbg(FYI, "cifs_query_path_info: FALLBACK");
rc = CIFSSMBQPathInfoBasic(xid, tcon, full_path,
(FILE_BASIC_INFO*)data,
cifs_sb->local_nls, remap);
@@ -557,6 +563,10 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
(void*)data + sizeof(FILE_BASIC_INFO),
cifs_sb->local_nls, remap);
}
+ cifs_dbg(FYI, "cifs_query_path_info: FALLBACK returns %d", rc);
+ if (!rc) {
+ tcon->broken_qpath_info = true;
+ }

I have a bit of a problem with the above...

Trying to change behavior automatically like this is fraught with peril
and cifs.ko is already a horrid mess of this sort of thing. It's bad
enough that we have some existing fallbacks for -EOPNOTSUPP and
-EINVAL, but those errors are a bit more informative so we can
rationalize them a bit. Also we don't mark the server as broken based
on those errors (though maybe there's an argument to be made for doing
that).

Now however, you're adding one for -EIO. That error tends to be what we
get when an NT error code can't easily be translated to a POSIX error.
So now you might end up falling back and marking this server with
broken_qpath_info when the server might actually support those
infolevels just fine, but you hit some strange transient error.

I think it would be best to respin this set and add a new vers= value
(e.g. 1.0basic or something) that users can mount with to enable this
behavior. That way instead of bloating out the already horribly bloated
standard 1.0 codepaths, you could add a new set of smb_version ops and
have that enable the use of the more basic infolevels.

Yes, it means that users might have to read the manpage to figure this
out, but I think trying to do this automatically will prove quite
problematic.