| Message ID | 20231208004156.9612-1-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: validate scrub_speed_max sysfs string | expand |
On 2023/12/8 11:11, David Disseldorp wrote: > Fail the sysfs I/O on any trailing non-space characters. > > Signed-off-by: David Disseldorp <ddiss@suse.de> Reviewed-by: Qu Wenruo <wqu@suse.com> Although I have an unrelated idea. Since memparse() provides the @endptr, can we rewind the @endptr, so that we can check if the last valid charactor is suffix 'e'. Then reject it from btrfs size. I really don't think we need exabytes suffix for our scrub speed limit usage. Thanks, Qu > --- > fs/btrfs/sysfs.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c > index e6b51fb3ddc1e..4c4642ef7c5f0 100644 > --- a/fs/btrfs/sysfs.c > +++ b/fs/btrfs/sysfs.c > @@ -1783,6 +1783,9 @@ static ssize_t btrfs_devinfo_scrub_speed_max_store(struct kobject *kobj, > unsigned long long limit; > > limit = memparse(buf, &endptr); > + endptr = skip_spaces(endptr); > + if (*endptr != 0) > + return -EINVAL; > WRITE_ONCE(device->scrub_speed_max, limit); > return len; > }
On Mon, 11 Dec 2023 13:48:15 +1030, Qu Wenruo wrote: > On 2023/12/8 11:11, David Disseldorp wrote: > > Fail the sysfs I/O on any trailing non-space characters. > > > > Signed-off-by: David Disseldorp <ddiss@suse.de> > > Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks. > Although I have an unrelated idea. > > Since memparse() provides the @endptr, can we rewind the @endptr, so > that we can check if the last valid charactor is suffix 'e'. > Then reject it from btrfs size. Hmm we could do that, but then we'd also break hex parsing. Rejecting 'e' in the absence of a leading '0x' or '0X' would work, but it's a bit ugly. Cheers, David
On Mon, Dec 11, 2023 at 01:48:15PM +1030, Qu Wenruo wrote: > > > On 2023/12/8 11:11, David Disseldorp wrote: > > Fail the sysfs I/O on any trailing non-space characters. > > > > Signed-off-by: David Disseldorp <ddiss@suse.de> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Although I have an unrelated idea. > > Since memparse() provides the @endptr, can we rewind the @endptr, so > that we can check if the last valid charactor is suffix 'e'. > Then reject it from btrfs size. > > I really don't think we need exabytes suffix for our scrub speed limit > usage. I think nobody will intentionally use the 'e' exabyte suffix in this case but I don't want to add an exception to parsing the values. We'd have to document that, explain why it's not accepted, it's additional work for a case I still dont understand why it's so important. If the exabyte scale values are not properly parsed by memparse() due to simple_strtoull() as a workaround we can add our parser based on kstrtoull() or do such change in memparse() proper.
On Fri, Dec 08, 2023 at 11:41:56AM +1100, David Disseldorp wrote: > Fail the sysfs I/O on any trailing non-space characters. > > Signed-off-by: David Disseldorp <ddiss@suse.de> Added to misc-next a few days ago with updated changelog with reasoning based on the recent discussions why we want the memparse and suffixes. Thanks.
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index e6b51fb3ddc1e..4c4642ef7c5f0 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -1783,6 +1783,9 @@ static ssize_t btrfs_devinfo_scrub_speed_max_store(struct kobject *kobj, unsigned long long limit; limit = memparse(buf, &endptr); + endptr = skip_spaces(endptr); + if (*endptr != 0) + return -EINVAL; WRITE_ONCE(device->scrub_speed_max, limit); return len; }
Fail the sysfs I/O on any trailing non-space characters. Signed-off-by: David Disseldorp <ddiss@suse.de> --- fs/btrfs/sysfs.c | 3 +++ 1 file changed, 3 insertions(+)