[PATCH 0/3] Minor updates to sysfs

[PATCH 0/3] Minor updates to sysfs

[David Sterba]

Hi,

a less-than-handful set of fixes to sysfs, two sanity checks and one additional
locking preventing a pretty rare race.

David Sterba (3):
  btrfs: add read-only check to sysfs handler of features
  btrfs: add check to sysfs handler of label
  btrfs: sysfs: protect reading label by lock

 fs/btrfs/sysfs.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
2.7.1

[PATCH 1/3] btrfs: add read-only check to sysfs handler of features

[David Sterba]

From: David Sterba <dsterba@suse.cz>

We don't want to trigger the change on a read-only filesystem, similar
to what the label handler does.

Signed-off-by: David Sterba <dsterba@suse.cz>
---
 fs/btrfs/sysfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 539e7b5e3f86..6a6bb600b1ff 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -120,6 +120,9 @@ static ssize_t btrfs_feature_attr_store(struct kobject *kobj,
 	if (!fs_info)
 		return -EPERM;
 
+	if (fs_info->sb->s_flags & MS_RDONLY)
+		return -EROFS;
+
 	ret = kstrtoul(skip_spaces(buf), 0, &val);
 	if (ret)
 		return ret;
-- 
2.7.1

[PATCH 2/3] btrfs: add check to sysfs handler of label

[David Sterba]

Add a sanity check for the fs_info as we will dereference it, similar to
what the 'store features' handler does.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/sysfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 6a6bb600b1ff..3d14618ce54b 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -377,6 +377,9 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 	size_t p_len;
 
+	if (!fs_info)
+		return -EPERM;
+
 	if (fs_info->sb->s_flags & MS_RDONLY)
 		return -EROFS;
 
-- 
2.7.1

[PATCH 3/3] btrfs: sysfs: protect reading label by lock

[David Sterba]

If the label setting ioctl races with sysfs label handler, we could get
mixed result in the output, part old part new. We should either get the
old or new label. The chances to hit this race are low.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/sysfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 3d14618ce54b..7b0da1dcb6df 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -367,7 +367,12 @@ static ssize_t btrfs_label_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 	char *label = fs_info->super_copy->label;
-	return snprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);
+
+	spin_lock(&fs_info->super_lock);
+	snprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);
+	spin_unlock(&fs_info->super_lock);
+
+	return buf;
 }
 
 static ssize_t btrfs_label_store(struct kobject *kobj,
-- 
2.7.1

Re: [PATCH 3/3] btrfs: sysfs: protect reading label by lock

[Filipe Manana]

On Tue, Apr 26, 2016 at 3:32 PM, David Sterba <dsterba@suse.com> wrote:
> If the label setting ioctl races with sysfs label handler, we could get
> mixed result in the output, part old part new. We should either get the
> old or new label. The chances to hit this race are low.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/sysfs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 3d14618ce54b..7b0da1dcb6df 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -367,7 +367,12 @@ static ssize_t btrfs_label_show(struct kobject *kobj,
>  {
>         struct btrfs_fs_info *fs_info = to_fs_info(kobj);
>         char *label = fs_info->super_copy->label;
> -       return snprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);
> +
> +       spin_lock(&fs_info->super_lock);
> +       snprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);
> +       spin_unlock(&fs_info->super_lock);
> +
> +       return buf;

We should return a ssize_t value, not a char *. I.e. return what
snprintf returns. This should make gcc emit a warning.

>  }
>
>  static ssize_t btrfs_label_store(struct kobject *kobj,
> --
> 2.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 3/3] btrfs: sysfs: protect reading label by lock

[David Sterba]

On Tue, Apr 26, 2016 at 04:52:09PM +0100, Filipe Manana wrote:
> On Tue, Apr 26, 2016 at 3:32 PM, David Sterba <dsterba@suse.com> wrote:
> > If the label setting ioctl races with sysfs label handler, we could get
> > mixed result in the output, part old part new. We should either get the
> > old or new label. The chances to hit this race are low.
> >
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >  fs/btrfs/sysfs.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> > index 3d14618ce54b..7b0da1dcb6df 100644
> > --- a/fs/btrfs/sysfs.c
> > +++ b/fs/btrfs/sysfs.c
> > @@ -367,7 +367,12 @@ static ssize_t btrfs_label_show(struct kobject *kobj,
> >  {
> >         struct btrfs_fs_info *fs_info = to_fs_info(kobj);
> >         char *label = fs_info->super_copy->label;
> > -       return snprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);
> > +
> > +       spin_lock(&fs_info->super_lock);
> > +       snprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);
> > +       spin_unlock(&fs_info->super_lock);
> > +
> > +       return buf;
> 
> We should return a ssize_t value, not a char *. I.e. return what
> snprintf returns. This should make gcc emit a warning.

Indeed the warning was there and I overlooked it, last minute patches
before leaving. Thanks for catching it.