Skip to contentSkip to conversation

Minor updates to sysfs

PATCH In review David Sterba 3 participants 6 messages Apr 26, 2016 Btrfs file system

lore
Share & tools
lore

David Sterba

author maintainer
#0 lore raw

cover Minor updates to sysfs

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(-)
Signature · 1 line
2.7.1
author maintainer
#1 lore raw

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

From: David Sterba <[email protected]>

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 <[email protected]>
---
 fs/btrfs/sysfs.c | 3 +++
 1 file changed, 3 insertions(+)
fs/btrfs/sysfs.c +3 -0
index 539e7b5e3f86..6a6bb600b1ff 100644
@@ -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;
Signature · 1 line
2.7.1
author maintainer
#2 lore raw

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

In reply to 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 <[email protected]>
---
 fs/btrfs/sysfs.c | 3 +++
 1 file changed, 3 insertions(+)
fs/btrfs/sysfs.c +3 -0
index 6a6bb600b1ff..3d14618ce54b 100644
@@ -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;
Signature · 1 line
2.7.1
author maintainer
#3 lore raw

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

In reply to 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 <[email protected]>
---
 fs/btrfs/sysfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/btrfs/sysfs.c +6 -1
index 3d14618ce54b..7b0da1dcb6df 100644
@@ -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,
Signature · 1 line
2.7.1
#4 lore raw

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

Quoted 14 lines · 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 <[email protected]>
---
 fs/btrfs/sysfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/btrfs/sysfs.c +0 -0
index 3d14618ce54b..7b0da1dcb6df 100644

fs/btrfs/sysfs.c static ssize_t btrfs_label_show(struct kobject *kobj, · line 375

Quoted 10 lines
fs/btrfs/sysfs.c +6 -1
@@ -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.

Quoted 10 lines
fs/btrfs/sysfs.c +0 -1
 }
 
 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 5 lines
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."
#5 lore raw

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

Quoted 20 lines · Filipe Manana
Quoted 19 lines · 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 <[email protected]>
---
 fs/btrfs/sysfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/btrfs/sysfs.c +1 -1
index 3d14618ce54b..7b0da1dcb6df 100644
@@ -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);
+
Quoted 8 lines

fs/btrfs/sysfs.c static ssize_t btrfs_label_show(struct kobject *kobj, · line 375

Quoted 5 lines
fs/btrfs/sysfs.c +5 -0
+       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.