cifs: fix return value for cifs_listxattr

Message ID 20181025054336.7348-1-lsahlber@redhat.com
State New
Headers show
Series
  • cifs: fix return value for cifs_listxattr
Related show

Commit Message

Ronnie Sahlberg Oct. 25, 2018, 5:43 a.m.
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(-)

Comments

Aurélien Aptel Oct. 25, 2018, 12:55 p.m. | #1
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,
Steve French Oct. 25, 2018, 4:58 p.m. | #2
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
>
ronnie sahlberg Oct. 25, 2018, 8:10 p.m. | #3
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

Patch

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;