| Message ID | 20181025054336.7348-1-lsahlber@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: fix return value for cifs_listxattr | expand |
Ronnie Sahlberg <lsahlber@redhat.com> writes: > If the application buffer was too small to fit all the names > we would still count the number of bytes and return this for > listxattr. This would then trigger a BUG in usercopy.c > > Fix the computation of the size so that we return -ERANGE > correctly when the buffer is too small. > > This fixes the kernel BUG for xfstest generic/377 Good catch. Reviewed-by: Aurelien Aptel <aaptel@suse.com> Cheers,
Merged into cifs-2.6.git for-next. This is very exciting ... each xfstest that we fix gets us closer to the goal of getting all reasonable xfstests for network fs to pass ... and getting rid of noisy xfstest failures and skips makes it much more likely that we can catch regressions earlier. On Thu, Oct 25, 2018 at 12:43 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > If the application buffer was too small to fit all the names > we would still count the number of bytes and return this for > listxattr. This would then trigger a BUG in usercopy.c > > Fix the computation of the size so that we return -ERANGE > correctly when the buffer is too small. > > This fixes the kernel BUG for xfstest generic/377 > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/smb2ops.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index f85fc5aa2710..59dc9ae0ecfd 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -747,6 +747,7 @@ move_smb2_ea_to_cifs(char *dst, size_t dst_size, > int rc = 0; > unsigned int ea_name_len = ea_name ? strlen(ea_name) : 0; > char *name, *value; > + size_t buf_size = dst_size; > size_t name_len, value_len, user_name_len; > > while (src_size > 0) { > @@ -782,9 +783,10 @@ move_smb2_ea_to_cifs(char *dst, size_t dst_size, > /* 'user.' plus a terminating null */ > user_name_len = 5 + 1 + name_len; > > - rc += user_name_len; > - > - if (dst_size >= user_name_len) { > + if (buf_size == 0) { > + /* skip copy - calc size only */ > + rc += user_name_len; > + } else if (dst_size >= user_name_len) { > dst_size -= user_name_len; > memcpy(dst, "user.", 5); > dst += 5; > @@ -792,8 +794,7 @@ move_smb2_ea_to_cifs(char *dst, size_t dst_size, > dst += name_len; > *dst = 0; > ++dst; > - } else if (dst_size == 0) { > - /* skip copy - calc size only */ > + rc += user_name_len; > } else { > /* stop before overrun buffer */ > rc = -ERANGE; > -- > 2.13.6 >
The test still fails, but it does not trigger a BUG or oops. The failure is the same case sensitive/case-insensitive issue for the attributes as we have in a lot of the other tests. I.e. user.FOO vs user.foo. That brings the question. What/should we do about xattrs and case? Just mapping them straight to EAs like we do now does not provide correct xattr semantics. We could instead of just mapping the xattr name straight to an EA with the same name use a case-preserving mapping. Something like uuencoding the names before we store them as EAs. The drawback would then be that we would break compatibility with existing xattrs. I guess we could add a new mount option to enable/disable this new case-preserving mapping. Or, how much of an issue is this in real world? Maybe the only thing that actually cares about case-preserving xattrs are the xfstests? On Fri, Oct 26, 2018 at 2:59 AM Steve French <smfrench@gmail.com> wrote: > > Merged into cifs-2.6.git for-next. This is very exciting ... each > xfstest that we fix gets us closer to the goal of getting all > reasonable xfstests for network fs to pass ... and getting rid of > noisy xfstest failures and skips makes it much more likely that we can > catch regressions earlier. > On Thu, Oct 25, 2018 at 12:43 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > > > If the application buffer was too small to fit all the names > > we would still count the number of bytes and return this for > > listxattr. This would then trigger a BUG in usercopy.c > > > > Fix the computation of the size so that we return -ERANGE > > correctly when the buffer is too small. > > > > This fixes the kernel BUG for xfstest generic/377 > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/smb2ops.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > > index f85fc5aa2710..59dc9ae0ecfd 100644 > > --- a/fs/cifs/smb2ops.c > > +++ b/fs/cifs/smb2ops.c > > @@ -747,6 +747,7 @@ move_smb2_ea_to_cifs(char *dst, size_t dst_size, > > int rc = 0; > > unsigned int ea_name_len = ea_name ? strlen(ea_name) : 0; > > char *name, *value; > > + size_t buf_size = dst_size; > > size_t name_len, value_len, user_name_len; > > > > while (src_size > 0) { > > @@ -782,9 +783,10 @@ move_smb2_ea_to_cifs(char *dst, size_t dst_size, > > /* 'user.' plus a terminating null */ > > user_name_len = 5 + 1 + name_len; > > > > - rc += user_name_len; > > - > > - if (dst_size >= user_name_len) { > > + if (buf_size == 0) { > > + /* skip copy - calc size only */ > > + rc += user_name_len; > > + } else if (dst_size >= user_name_len) { > > dst_size -= user_name_len; > > memcpy(dst, "user.", 5); > > dst += 5; > > @@ -792,8 +794,7 @@ move_smb2_ea_to_cifs(char *dst, size_t dst_size, > > dst += name_len; > > *dst = 0; > > ++dst; > > - } else if (dst_size == 0) { > > - /* skip copy - calc size only */ > > + rc += user_name_len; > > } else { > > /* stop before overrun buffer */ > > rc = -ERANGE; > > -- > > 2.13.6 > > > > > -- > Thanks, > > Steve
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index f85fc5aa2710..59dc9ae0ecfd 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -747,6 +747,7 @@ move_smb2_ea_to_cifs(char *dst, size_t dst_size, int rc = 0; unsigned int ea_name_len = ea_name ? strlen(ea_name) : 0; char *name, *value; + size_t buf_size = dst_size; size_t name_len, value_len, user_name_len; while (src_size > 0) { @@ -782,9 +783,10 @@ move_smb2_ea_to_cifs(char *dst, size_t dst_size, /* 'user.' plus a terminating null */ user_name_len = 5 + 1 + name_len; - rc += user_name_len; - - if (dst_size >= user_name_len) { + if (buf_size == 0) { + /* skip copy - calc size only */ + rc += user_name_len; + } else if (dst_size >= user_name_len) { dst_size -= user_name_len; memcpy(dst, "user.", 5); dst += 5; @@ -792,8 +794,7 @@ move_smb2_ea_to_cifs(char *dst, size_t dst_size, dst += name_len; *dst = 0; ++dst; - } else if (dst_size == 0) { - /* skip copy - calc size only */ + rc += user_name_len; } else { /* stop before overrun buffer */ rc = -ERANGE;
If the application buffer was too small to fit all the names we would still count the number of bytes and return this for listxattr. This would then trigger a BUG in usercopy.c Fix the computation of the size so that we return -ERANGE correctly when the buffer is too small. This fixes the kernel BUG for xfstest generic/377 Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/smb2ops.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)