xfs_{compat_,}attrmulti_by_handle could return an errno with incorrect
sign in some cases. While at it, make sure ENOMEM is returned instead of
E2BIG if kmalloc fails.
Signed-off-by: Tuomas Tynkkynen <[email protected]>
---
Compile tested only. For the ERANGE case, I also wonder if it should
be assigning to ops[i].am_error instead of error, and/or have a break.
fs/xfs/xfs_ioctl.c | 5 +++--
fs/xfs/xfs_ioctl32.c | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 0b18776..2d8f4fd 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -543,10 +543,11 @@ xfs_attrmulti_by_handle(
ops = memdup_user(am_hreq.ops, size);
if (IS_ERR(ops)) {
- error = PTR_ERR(ops);
+ error = -PTR_ERR(ops);
goto out_dput;
}
+ error = ENOMEM;
attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
if (!attr_name)
goto out_kfree_ops;
@@ -556,7 +557,7 @@ xfs_attrmulti_by_handle(
ops[i].am_error = strncpy_from_user((char *)attr_name,
ops[i].am_attrname, MAXNAMELEN);
if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
- error = -ERANGE;
+ error = ERANGE;
if (ops[i].am_error < 0)
break;
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index a7992f8..944d5ba 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -424,10 +424,11 @@ xfs_compat_attrmulti_by_handle(
ops = memdup_user(compat_ptr(am_hreq.ops), size);
if (IS_ERR(ops)) {
- error = PTR_ERR(ops);
+ error = -PTR_ERR(ops);
goto out_dput;
}
+ error = ENOMEM;
attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
if (!attr_name)
goto out_kfree_ops;
@@ -438,7 +439,7 @@ xfs_compat_attrmulti_by_handle(
compat_ptr(ops[i].am_attrname),
MAXNAMELEN);
if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
- error = -ERANGE;
+ error = ERANGE;
if (ops[i].am_error < 0)
break;
--
1.7.9.5
Hi Tuomas,
On 04/21 2014 18:04 PM, Tuomas Tynkkynen wrote:
> xfs_{compat_,}attrmulti_by_handle could return an errno with incorrect
> sign in some cases. While at it, make sure ENOMEM is returned instead of
> E2BIG if kmalloc fails.
>
> Signed-off-by: Tuomas Tynkkynen <[email protected]>
> ---
> Compile tested only. For the ERANGE case, I also wonder if it should
> be assigning to ops[i].am_error instead of error, and/or have a break.
If I understand right, ops[i].am_error is used to save the internal operation result,
i.e, xfs_attrmult_attr_get{set}... but error is used for the ioctl call results.
Therefore, assign ERANGE to error is compatible with the VFS set{get}xattr syscalls in
case of "if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)".
It seems that we don't need to have a break in this case because xfs_attrmulti_by_handle()
is used to process multiple attrs. Hence if a given attrname in ops array is invalid,
the am_error will indicate that with ENOATTR or EFAULT...but it should proceed to loop
through the left array members.
Also, looks there is another minor issue at xfs_attrmulti_attr_set(), we should return
-PTR_ERR(kbuf) if call memdup_user() failed.
>
> fs/xfs/xfs_ioctl.c | 5 +++--
> fs/xfs/xfs_ioctl32.c | 5 +++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 0b18776..2d8f4fd 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -543,10 +543,11 @@ xfs_attrmulti_by_handle(
>
> ops = memdup_user(am_hreq.ops, size);
> if (IS_ERR(ops)) {
> - error = PTR_ERR(ops);
> + error = -PTR_ERR(ops);
> goto out_dput;
> }
>
> + error = ENOMEM;
> attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
> if (!attr_name)
> goto out_kfree_ops;
> @@ -556,7 +557,7 @@ xfs_attrmulti_by_handle(
> ops[i].am_error = strncpy_from_user((char *)attr_name,
> ops[i].am_attrname, MAXNAMELEN);
> if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
> - error = -ERANGE;
> + error = ERANGE;
> if (ops[i].am_error < 0)
> break;
>
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index a7992f8..944d5ba 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -424,10 +424,11 @@ xfs_compat_attrmulti_by_handle(
>
> ops = memdup_user(compat_ptr(am_hreq.ops), size);
> if (IS_ERR(ops)) {
> - error = PTR_ERR(ops);
> + error = -PTR_ERR(ops);
> goto out_dput;
> }
>
> + error = ENOMEM;
> attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
> if (!attr_name)
> goto out_kfree_ops;
> @@ -438,7 +439,7 @@ xfs_compat_attrmulti_by_handle(
> compat_ptr(ops[i].am_attrname),
> MAXNAMELEN);
> if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
> - error = -ERANGE;
> + error = ERANGE;
> if (ops[i].am_error < 0)
> break;
>
Thanks,
-Jeff
On Mon, Apr 21, 2014 at 01:04:47PM +0300, Tuomas Tynkkynen wrote:
> xfs_{compat_,}attrmulti_by_handle could return an errno with incorrect
> sign in some cases. While at it, make sure ENOMEM is returned instead of
> E2BIG if kmalloc fails.
>
> Signed-off-by: Tuomas Tynkkynen <[email protected]>
> ---
The fix looks good to me. There has been talk recently of starting to
fix up our internal positive-to-negative error conversions to be more
consistent with the rest of the kernel. Given that, I wonder if it's
better here to go the other way. E.g., remove the negation in the
out_dput path and convert the positive error codes (and thus we don't
need to add more negative-to-positive-to-negative conversions here).
Thoughts?
Brian
> Compile tested only. For the ERANGE case, I also wonder if it should
> be assigning to ops[i].am_error instead of error, and/or have a break.
>
> fs/xfs/xfs_ioctl.c | 5 +++--
> fs/xfs/xfs_ioctl32.c | 5 +++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 0b18776..2d8f4fd 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -543,10 +543,11 @@ xfs_attrmulti_by_handle(
>
> ops = memdup_user(am_hreq.ops, size);
> if (IS_ERR(ops)) {
> - error = PTR_ERR(ops);
> + error = -PTR_ERR(ops);
> goto out_dput;
> }
>
> + error = ENOMEM;
> attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
> if (!attr_name)
> goto out_kfree_ops;
> @@ -556,7 +557,7 @@ xfs_attrmulti_by_handle(
> ops[i].am_error = strncpy_from_user((char *)attr_name,
> ops[i].am_attrname, MAXNAMELEN);
> if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
> - error = -ERANGE;
> + error = ERANGE;
> if (ops[i].am_error < 0)
> break;
>
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index a7992f8..944d5ba 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -424,10 +424,11 @@ xfs_compat_attrmulti_by_handle(
>
> ops = memdup_user(compat_ptr(am_hreq.ops), size);
> if (IS_ERR(ops)) {
> - error = PTR_ERR(ops);
> + error = -PTR_ERR(ops);
> goto out_dput;
> }
>
> + error = ENOMEM;
> attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
> if (!attr_name)
> goto out_kfree_ops;
> @@ -438,7 +439,7 @@ xfs_compat_attrmulti_by_handle(
> compat_ptr(ops[i].am_attrname),
> MAXNAMELEN);
> if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
> - error = -ERANGE;
> + error = ERANGE;
> if (ops[i].am_error < 0)
> break;
>
> --
> 1.7.9.5
>
> _______________________________________________
> xfs mailing list
> [email protected]
> http://oss.sgi.com/mailman/listinfo/xfs
On Mon, Apr 21, 2014 at 08:46:39PM +0800, Jeff Liu wrote:
> Hi Tuomas,
>
> On 04/21 2014 18:04 PM, Tuomas Tynkkynen wrote:
> > xfs_{compat_,}attrmulti_by_handle could return an errno with incorrect
> > sign in some cases. While at it, make sure ENOMEM is returned instead of
> > E2BIG if kmalloc fails.
> >
> > Signed-off-by: Tuomas Tynkkynen <[email protected]>
> > ---
> > Compile tested only. For the ERANGE case, I also wonder if it should
> > be assigning to ops[i].am_error instead of error, and/or have a break.
>
> If I understand right, ops[i].am_error is used to save the internal operation result,
> i.e, xfs_attrmult_attr_get{set}... but error is used for the ioctl call results.
> Therefore, assign ERANGE to error is compatible with the VFS set{get}xattr syscalls in
> case of "if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)".
>
But we set 'error' in this case and effectively try to continue the operation
whereas the traditional vfs path returns...
> It seems that we don't need to have a break in this case because xfs_attrmulti_by_handle()
> is used to process multiple attrs. Hence if a given attrname in ops array is invalid,
> the am_error will indicate that with ENOATTR or EFAULT...but it should proceed to loop
> through the left array members.
>
Perhaps so if am_error == 0, but it depends on what attr_name contains
at that point (stale data?). Otherwise, we try to proceed with a
truncated name. It looks like the consistent thing to do would be set
am_error to ERANGE and continue (i.e., skip the op and move on to the
next).
Brian
> Also, looks there is another minor issue at xfs_attrmulti_attr_set(), we should return
> -PTR_ERR(kbuf) if call memdup_user() failed.
>
> >
> > fs/xfs/xfs_ioctl.c | 5 +++--
> > fs/xfs/xfs_ioctl32.c | 5 +++--
> > 2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 0b18776..2d8f4fd 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -543,10 +543,11 @@ xfs_attrmulti_by_handle(
> >
> > ops = memdup_user(am_hreq.ops, size);
> > if (IS_ERR(ops)) {
> > - error = PTR_ERR(ops);
> > + error = -PTR_ERR(ops);
> > goto out_dput;
> > }
> >
> > + error = ENOMEM;
> > attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
> > if (!attr_name)
> > goto out_kfree_ops;
> > @@ -556,7 +557,7 @@ xfs_attrmulti_by_handle(
> > ops[i].am_error = strncpy_from_user((char *)attr_name,
> > ops[i].am_attrname, MAXNAMELEN);
> > if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
> > - error = -ERANGE;
> > + error = ERANGE;
> > if (ops[i].am_error < 0)
> > break;
> >
> > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> > index a7992f8..944d5ba 100644
> > --- a/fs/xfs/xfs_ioctl32.c
> > +++ b/fs/xfs/xfs_ioctl32.c
> > @@ -424,10 +424,11 @@ xfs_compat_attrmulti_by_handle(
> >
> > ops = memdup_user(compat_ptr(am_hreq.ops), size);
> > if (IS_ERR(ops)) {
> > - error = PTR_ERR(ops);
> > + error = -PTR_ERR(ops);
> > goto out_dput;
> > }
> >
> > + error = ENOMEM;
> > attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
> > if (!attr_name)
> > goto out_kfree_ops;
> > @@ -438,7 +439,7 @@ xfs_compat_attrmulti_by_handle(
> > compat_ptr(ops[i].am_attrname),
> > MAXNAMELEN);
> > if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
> > - error = -ERANGE;
> > + error = ERANGE;
> > if (ops[i].am_error < 0)
> > break;
> >
>
> Thanks,
> -Jeff
>
> _______________________________________________
> xfs mailing list
> [email protected]
> http://oss.sgi.com/mailman/listinfo/xfs
On 04/21 2014 21:09 PM, Brian Foster wrote:
> On Mon, Apr 21, 2014 at 08:46:39PM +0800, Jeff Liu wrote:
>> Hi Tuomas,
>>
>> On 04/21 2014 18:04 PM, Tuomas Tynkkynen wrote:
>>> xfs_{compat_,}attrmulti_by_handle could return an errno with incorrect
>>> sign in some cases. While at it, make sure ENOMEM is returned instead of
>>> E2BIG if kmalloc fails.
>>>
>>> Signed-off-by: Tuomas Tynkkynen <[email protected]>
>>> ---
>>> Compile tested only. For the ERANGE case, I also wonder if it should
>>> be assigning to ops[i].am_error instead of error, and/or have a break.
>>
>> If I understand right, ops[i].am_error is used to save the internal operation result,
>> i.e, xfs_attrmult_attr_get{set}... but error is used for the ioctl call results.
>> Therefore, assign ERANGE to error is compatible with the VFS set{get}xattr syscalls in
>> case of "if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)".
>>
>
> But we set 'error' in this case and effectively try to continue the operation
> whereas the traditional vfs path returns...
So the error would always be set to ERANGE if one or more attrname is/are invalid.
>
>> It seems that we don't need to have a break in this case because xfs_attrmulti_by_handle()
>> is used to process multiple attrs. Hence if a given attrname in ops array is invalid,
>> the am_error will indicate that with ENOATTR or EFAULT...but it should proceed to loop
>> through the left array members.
>>
>
> Perhaps so if am_error == 0, but it depends on what attr_name contains
> at that point (stale data?). Otherwise, we try to proceed with a
> truncated name. It looks like the consistent thing to do would be set
> am_error to ERANGE and continue (i.e., skip the op and move on to the
> next).
If we continue to process a truncated name in case of MAXNAMELEN, it would return
EFAULT for SET/REMOVE operations, and ENOATTR for GET operation, which would be
set back to am_error, but error still keep as ERANGE which is consistently.
Thanks,
-Jeff
On Mon, Apr 21, 2014 at 10:01:34PM +0800, Jeff Liu wrote:
>
> On 04/21 2014 21:09 PM, Brian Foster wrote:
> > On Mon, Apr 21, 2014 at 08:46:39PM +0800, Jeff Liu wrote:
> >> Hi Tuomas,
> >>
> >> On 04/21 2014 18:04 PM, Tuomas Tynkkynen wrote:
> >>> xfs_{compat_,}attrmulti_by_handle could return an errno with incorrect
> >>> sign in some cases. While at it, make sure ENOMEM is returned instead of
> >>> E2BIG if kmalloc fails.
> >>>
> >>> Signed-off-by: Tuomas Tynkkynen <[email protected]>
> >>> ---
> >>> Compile tested only. For the ERANGE case, I also wonder if it should
> >>> be assigning to ops[i].am_error instead of error, and/or have a break.
> >>
> >> If I understand right, ops[i].am_error is used to save the internal operation result,
> >> i.e, xfs_attrmult_attr_get{set}... but error is used for the ioctl call results.
> >> Therefore, assign ERANGE to error is compatible with the VFS set{get}xattr syscalls in
> >> case of "if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)".
> >>
> >
> > But we set 'error' in this case and effectively try to continue the operation
> > whereas the traditional vfs path returns...
>
> So the error would always be set to ERANGE if one or more attrname is/are invalid.
>
Right...
> >
> >> It seems that we don't need to have a break in this case because xfs_attrmulti_by_handle()
> >> is used to process multiple attrs. Hence if a given attrname in ops array is invalid,
> >> the am_error will indicate that with ENOATTR or EFAULT...but it should proceed to loop
> >> through the left array members.
> >>
> >
> > Perhaps so if am_error == 0, but it depends on what attr_name contains
> > at that point (stale data?). Otherwise, we try to proceed with a
> > truncated name. It looks like the consistent thing to do would be set
> > am_error to ERANGE and continue (i.e., skip the op and move on to the
> > next).
>
> If we continue to process a truncated name in case of MAXNAMELEN, it would return
> EFAULT for SET/REMOVE operations, and ENOATTR for GET operation, which would be
> set back to am_error, but error still keep as ERANGE which is consistently.
>
Ah, we should hit an error in the xfs_attr_name_to_xname() call
deeper in the callchain, so the truncated name case should not be an
issue. The am_error == 0 case still looks weird to me.
So we return an error and set am_error if there's an issue with the attr
name. If there's another error in the op path, we set am_error only and
continue on. Not being familiar with the interface, I'm curious if there
is any particular reason for that difference in behavior. It seems like
the caller should process all of the return codes anyways.
Brian
>
> Thanks,
> -Jeff
>
> _______________________________________________
> xfs mailing list
> [email protected]
> http://oss.sgi.com/mailman/listinfo/xfs
On 04/12 2014 22:36 PM, Brian Foster wrote:
> On Mon, Apr 21, 2014 at 10:01:34PM +0800, Jeff Liu wrote:
>>
>> On 04/21 2014 21:09 PM, Brian Foster wrote:
>>> On Mon, Apr 21, 2014 at 08:46:39PM +0800, Jeff Liu wrote:
>>>> Hi Tuomas,
>>>>
>>>> On 04/21 2014 18:04 PM, Tuomas Tynkkynen wrote:
>>>>> xfs_{compat_,}attrmulti_by_handle could return an errno with incorrect
>>>>> sign in some cases. While at it, make sure ENOMEM is returned instead of
>>>>> E2BIG if kmalloc fails.
>>>>>
>>>>> Signed-off-by: Tuomas Tynkkynen <[email protected]>
>>>>> ---
>>>>> Compile tested only. For the ERANGE case, I also wonder if it should
>>>>> be assigning to ops[i].am_error instead of error, and/or have a break.
>>>>
>>>> If I understand right, ops[i].am_error is used to save the internal operation result,
>>>> i.e, xfs_attrmult_attr_get{set}... but error is used for the ioctl call results.
>>>> Therefore, assign ERANGE to error is compatible with the VFS set{get}xattr syscalls in
>>>> case of "if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)".
>>>>
>>>
>>> But we set 'error' in this case and effectively try to continue the operation
>>> whereas the traditional vfs path returns...
>>
>> So the error would always be set to ERANGE if one or more attrname is/are invalid.
>>
>
> Right...
>
>>>
>>>> It seems that we don't need to have a break in this case because xfs_attrmulti_by_handle()
>>>> is used to process multiple attrs. Hence if a given attrname in ops array is invalid,
>>>> the am_error will indicate that with ENOATTR or EFAULT...but it should proceed to loop
>>>> through the left array members.
>>>>
>>>
>>> Perhaps so if am_error == 0, but it depends on what attr_name contains
>>> at that point (stale data?). Otherwise, we try to proceed with a
>>> truncated name. It looks like the consistent thing to do would be set
>>> am_error to ERANGE and continue (i.e., skip the op and move on to the
>>> next).
>>
>> If we continue to process a truncated name in case of MAXNAMELEN, it would return
>> EFAULT for SET/REMOVE operations, and ENOATTR for GET operation, which would be
>> set back to am_error, but error still keep as ERANGE which is consistently.
>>
>
> Ah, we should hit an error in the xfs_attr_name_to_xname() call
> deeper in the callchain, so the truncated name case should not be an
> issue. The am_error == 0 case still looks weird to me.
IMO am_error == 0 case means that the supplied attrname is NULL, and this would be
detected by xfs_attr_name_to_xname() as well, i.e,
if (!aname)
return EINVAL;
>
> So we return an error and set am_error if there's an issue with the attr
> name.
But the am_error would be replaced with another errno if any operation
is failed thereafter, or 0 on success. So the error is only used to indicate
the ioctl call status anyway.
> If there's another error in the op path, we set am_error only and
> continue on. Not being familiar with the interface, I'm curious if there
> is any particular reason for that difference in behavior. It seems like
> the caller should process all of the return codes anyways.
It seems to me that the design is convenient to the user space as the user could
get more precise call status for one/more particular attrname{s} operations.
Thanks,
-Jeff