btrfs: label should not contain return char

PATCH Superseded Anand Jain 7 participants 30 messages May 19, 2014 – Jul 1, 2014 Btrfs file system

lore

Share & tools

lore

Anand Jain

author

2014-05-19 17:01 UTC #0 lore raw

PATCH 1/2 btrfs: label should not contain return char

From: Anand Jain <[email protected]>

generally if you use
echo "test" > /sys/fs/btrfs/<fsid>/label
it would introduce return char at the end and it can not be part of the label. The correct command is
echo -n "test" > /sys/fs/btrfs/<fsid>/label

This patch will check for this user error

Signed-off-by Anand Jain <[email protected]>

---
 fs/btrfs/sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/btrfs/sysfs.c +1 -1

index c5eb214..63c2907 100644
@@ -374,7 +374,7 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_root *root = fs_info->fs_root;
 	int ret;
 
-	if (len >= BTRFS_LABEL_SIZE) {
+	if (len >= BTRFS_LABEL_SIZE || strchr(buf, '\n')) {
 		pr_err("BTRFS: unable to set label with more than %d bytes\n",
 		       BTRFS_LABEL_SIZE - 1);
 		return -EINVAL;

Signature · 1 line

1.8.5.3

Anand Jain

author

2014-05-19 17:01 UTC #1 lore raw

PATCH 2/2 btrfs: usage error should not be logged into system log

From: Anand Jain <[email protected]>

I have an opinion that system logs /var/log/messages are valuable info to investigate the real system issues at the data center. People handling data center issues do spend a lot time and efforts analyzing messages files. Having usage error logged into /var/log/messages is something we should avoid.

Signed-off-by Anand Jain <[email protected]>

---
 fs/btrfs/sysfs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

fs/btrfs/sysfs.c +1 -4

index 63c2907..f729199 100644
@@ -374,11 +374,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_root *root = fs_info->fs_root;
 	int ret;
 
-	if (len >= BTRFS_LABEL_SIZE || strchr(buf, '\n')) {
-		pr_err("BTRFS: unable to set label with more than %d bytes\n",
-		       BTRFS_LABEL_SIZE - 1);
+	if (len >= BTRFS_LABEL_SIZE || strchr(buf, '\n'))
 		return -EINVAL;
-	}
 
 	trans = btrfs_start_transaction(root, 0);
 	if (IS_ERR(trans))

Signature · 1 line

1.8.5.3

Anand Jain

author

2014-05-20 06:35 UTC #2 lore raw

PATCH v2 2/2 btrfs: usage error should not be logged into system log

From: Anand Jain <[email protected]>

Signed-off-by Anand Jain <[email protected]>

---
v2: rebase

 fs/btrfs/sysfs.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

fs/btrfs/sysfs.c +0 -2

index ca63fcd..36fae0f 100644
@@ -385,8 +385,6 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 		*pos = '\0';
 
 	if (strlen(label) >= BTRFS_LABEL_SIZE) {
-		pr_err("BTRFS: unable to set label with more than %d bytes\n",
-		       BTRFS_LABEL_SIZE - 1);
 		kfree(label);
 		return -EINVAL;
 	}

Signature · 1 line

1.7.1

David Sterba

Reviewed-by on v2 2/2

2014-05-20 16:36 UTC #3 lore raw

Re: [PATCH 2/2 v2] btrfs: usage error should not be logged into system log

Quoted 10 lines · Anand Jain

From: Anand Jain <[email protected]>
I have an opinion that system logs /var/log/messages are valuable info to investigate the real system issues at the data center. People handling data center issues do spend a lot time and efforts analyzing messages files. Having usage error logged into /var/log/messages is something we should avoid.
Signed-off-by Anand Jain <[email protected]>

Reviewed-by David Sterba <[email protected]>

In this case the source of the error is only one and the label constraints are simple, so the syslog message does not add much.

Eric Sandeen

2014-05-19 17:16 UTC #4 lore raw

In reply to Anand Jain

Quoted 9 lines · Anand Jain

From: Anand Jain <[email protected]>
generally if you use
echo "test" > /sys/fs/btrfs/<fsid>/label
it would introduce return char at the end and it can not be part of the label. The correct command is
echo -n "test" > /sys/fs/btrfs/<fsid>/label
This patch will check for this user error

Wouldn't it be a lot better to just strip the "\n" if it exists?

Quoted 10 lines

Signed-off-by Anand Jain <[email protected]>
---
 fs/btrfs/sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/btrfs/sysfs.c +0 -0
index c5eb214..63c2907 100644

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

Quoted 7 lines

fs/btrfs/sysfs.c +1 -1

@@ -374,7 +374,7 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_root *root = fs_info->fs_root;
 	int ret;
 
-	if (len >= BTRFS_LABEL_SIZE) {
+	if (len >= BTRFS_LABEL_SIZE || strchr(buf, '\n')) {
 		pr_err("BTRFS: unable to set label with more than %d bytes\n",
 		       BTRFS_LABEL_SIZE - 1);

so if I do:

# echo "mylabel" > /sys/fs/btrfs/<fsid>/label

I'll get:

BTRFS: unable to set label with more than 255 bytes"

which would be pretty confusing, IMHO, given the short label I tried to create.

Just strip out the \n ...

-Eric

Quoted 2 lines

fs/btrfs/sysfs.c +0 -0
 		return -EINVAL;

Anand Jain

author

2014-05-20 06:37 UTC #5 lore raw

Quoted 9 lines · Eric Sandeen

Quoted 8 lines · Anand Jain
From: Anand Jain <[email protected]>
generally if you use
echo "test" > /sys/fs/btrfs/<fsid>/label
it would introduce return char at the end and it can not be part of the label. The correct command is
echo -n "test" > /sys/fs/btrfs/<fsid>/label

Quoted 4 lines

Quoted 1 line
This patch will check for this user error
Wouldn't it be a lot better to just strip the "\n" if it exists?

yes. Thanks Eric.

Quoted 41 lines

Quoted 10 lines
Signed-off-by Anand Jain <[email protected]>
---
  fs/btrfs/sysfs.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
fs/btrfs/sysfs.c +0 -0
index c5eb214..63c2907 100644
fs/btrfs/sysfs.c static ssize_t btrfs_label_store(struct kobject *kobj, · line 379
Quoted 7 lines
fs/btrfs/sysfs.c +1 -1
@@ -374,7 +374,7 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
  	struct btrfs_root *root = fs_info->fs_root;
  	int ret;
 
-	if (len >= BTRFS_LABEL_SIZE) {
+	if (len >= BTRFS_LABEL_SIZE || strchr(buf, '\n')) {
  		pr_err("BTRFS: unable to set label with more than %d bytes\n",
  		       BTRFS_LABEL_SIZE - 1);
so if I do:
# echo "mylabel" > /sys/fs/btrfs/<fsid>/label
I'll get:
BTRFS: unable to set label with more than 255 bytes"
which would be pretty confusing, IMHO, given the short label I tried to create.
Just strip out the \n ...
-Eric
fs/btrfs/sysfs.c static ssize_t btrfs_label_store(struct kobject *kobj, · line 380
Quoted 2 lines
fs/btrfs/sysfs.c +0 -0
  		return -EINVAL;
--
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

Roman Mamedov

2014-05-19 17:19 UTC #6 lore raw

In reply to Anand Jain

Quoted 9 lines · On Tue, 20 May 2014 01:04:30 +0800 Anand Jain <[email protected]> wrote:

From: Anand Jain <[email protected]>
generally if you use
echo "test" > /sys/fs/btrfs/<fsid>/label
it would introduce return char at the end and it can not be part of the label. The correct command is
echo -n "test" > /sys/fs/btrfs/<fsid>/label
This patch will check for this user error

Maybe instead consider checking for one trailing "\n", and silently remove it if passed, so that both of the mentioned variants of 'echo' can be used?

All other sysfs files do not care if you pass an extra "\n" at the end, e.g.

echo cfq > /sys/block/sda/queue/scheduler

works fine, doesn't require you to use "echo -n cfq".

Signature · 2 lines

With respect,
Roman

Anand Jain

author

2014-05-20 06:39 UTC #7 lore raw

Quoted 13 lines · Roman Mamedov

Quoted 9 lines · On Tue, 20 May 2014 01:04:30 +0800 Anand Jain <[email protected]> wrote:
From: Anand Jain <[email protected]>
generally if you use
echo "test" > /sys/fs/btrfs/<fsid>/label
it would introduce return char at the end and it can not be part of the label. The correct command is
echo -n "test" > /sys/fs/btrfs/<fsid>/label
This patch will check for this user error

Quoted 9 lines

Maybe instead consider checking for one trailing "\n", and silently remove it if passed, so that both of the mentioned variants of 'echo' can be used?
All other sysfs files do not care if you pass an extra "\n" at the end, e.g.
echo cfq > /sys/block/sda/queue/scheduler
works fine, doesn't require you to use "echo -n cfq".

Thanks Roman.

Anand Jain

author

2014-05-20 06:34 UTC #8 lore raw

PATCH v2 1/2 btrfs: label should not contain return char

In reply to Anand Jain

From: Anand Jain <[email protected]>

This patch will check for this user error

Signed-off-by Anand Jain <[email protected]>

---
v2: accepts review comments. Thanks Eric and Roman

 fs/btrfs/sysfs.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

fs/btrfs/sysfs.c +17 -3

index c5eb214..ca63fcd 100644
@@ -373,22 +373,36 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root = fs_info->fs_root;
 	int ret;
+	char *label;
+	char *pos;
 
-	if (len >= BTRFS_LABEL_SIZE) {
+	label = kzalloc(len, GFP_NOFS);
+	if (!label)
+		return -ENOMEM;
+
+	memcpy(label, buf, len);
+	if ((pos = strchr(label, '\n')))
+		*pos = '\0';
+
+	if (strlen(label) >= BTRFS_LABEL_SIZE) {
 		pr_err("BTRFS: unable to set label with more than %d bytes\n",
 		       BTRFS_LABEL_SIZE - 1);
+		kfree(label);
 		return -EINVAL;
 	}
 
 	trans = btrfs_start_transaction(root, 0);
-	if (IS_ERR(trans))
+	if (IS_ERR(trans)) {
+		kfree(label);
 		return PTR_ERR(trans);
+	}
 
 	spin_lock(&root->fs_info->super_lock);
-	strcpy(fs_info->super_copy->label, buf);
+	strcpy(fs_info->super_copy->label, label);
 	spin_unlock(&root->fs_info->super_lock);
 	ret = btrfs_commit_transaction(trans, root);
 
+	kfree(label);
 	if (!ret)
 		return len;

Signature · 1 line

1.7.1

Eric Sandeen

2014-05-20 16:32 UTC #9 lore raw

Quoted 24 lines · Anand Jain

From: Anand Jain <[email protected]>
generally if you use
echo "test" > /sys/fs/btrfs/<fsid>/label
it would introduce return char at the end and it can not be part of the label. The correct command is
echo -n "test" > /sys/fs/btrfs/<fsid>/label
This patch will check for this user error
Signed-off-by Anand Jain <[email protected]>
---
v2: accepts review comments. Thanks Eric and Roman
 fs/btrfs/sysfs.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)
fs/btrfs/sysfs.c +0 -0
index c5eb214..ca63fcd 100644
@@ -373,22 +373,36 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root = fs_info->fs_root;

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

Quoted 10 lines

fs/btrfs/sysfs.c +7 -1

 	int ret;
+	char *label;
+	char *pos;
 
-	if (len >= BTRFS_LABEL_SIZE) {
+	label = kzalloc(len, GFP_NOFS);
+	if (!label)
+		return -ENOMEM;
+
+	memcpy(label, buf, len);

+ strlcpy(label, buf, len);

would ensure that the resulting string is null-terminated... I don't know if "buf" comes in 0-terminated or not, or if len includes \0. *shrug* these are strings after all...

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

Quoted 2 lines

fs/btrfs/sysfs.c +2 -0

+	if ((pos = strchr(label, '\n')))
+		*pos = '\0';

label = strstrip(label); might be simpler/better?

(this would strip all leading & trailing whitespace, I presume that'd be desirable, but then maybe someone really does want " mylabel \t " ?)

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

Quoted 2 lines

fs/btrfs/sysfs.c +2 -0

+
+	if (strlen(label) >= BTRFS_LABEL_SIZE) {

hm, strlen doesn't include the \0 right? so if we had 256 chars + \0, this would pass, and the subsequent strcpy will copy 257 bytes into a 256-byte buffer, right?

(I'm terrible at string handling in C, so I might be wrong... you all can point and laugh if I am)

Quoted 10 lines

fs/btrfs/sysfs.c +3 -1

 		pr_err("BTRFS: unable to set label with more than %d bytes\n",
 		       BTRFS_LABEL_SIZE - 1);
+		kfree(label);
 		return -EINVAL;
 	}
 
 	trans = btrfs_start_transaction(root, 0);
-	if (IS_ERR(trans))
+	if (IS_ERR(trans)) {
+		kfree(label);

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

Quoted 10 lines

fs/btrfs/sysfs.c +3 -1

 		return PTR_ERR(trans);
+	}
 
 	spin_lock(&root->fs_info->super_lock);
-	strcpy(fs_info->super_copy->label, buf);
+	strcpy(fs_info->super_copy->label, label);
 	spin_unlock(&root->fs_info->super_lock);
 	ret = btrfs_commit_transaction(trans, root);
 
+	kfree(label);

after the 3rd kfree() maybe an

out:

target would be better....

(Random aside: why does btrfs support online fs relabeling, anyway?)

-Eric

Quoted 4 lines

fs/btrfs/sysfs.c +0 -0

 	if (!ret)
 		return len;

Anand Jain

author

2014-05-22 02:02 UTC #10 lore raw

Quoted 5 lines

(Random aside: why does btrfs support online fs relabeling, anyway?)
-Eric

Online you mean when mounted ?

But I had an opinion that should we support label store from the sysfs interface when the (sysfs) interface can't communicate the module's specific errors back to the user.?

Thanks

Eric Sandeen

2014-05-22 02:14 UTC #11 lore raw

Quoted 7 lines · Anand Jain

Quoted 5 lines
(Random aside: why does btrfs support online fs relabeling, anyway?)
-Eric
Online you mean when mounted ?

Yep - I'm just not sure who would ever want to do that.

Aren't labels primarly used for mounting, during the mount process?

So changing it while mounted seems like a feature looking for a usecase...

-Eric

Roman Mamedov

2014-05-22 04:14 UTC #12 lore raw

Quoted 9 lines · On Wed, 21 May 2014 21:14:07 -0500 Eric Sandeen <[email protected]> wrote:

Quoted 5 lines
Quoted 3 lines
(Random aside: why does btrfs support online fs relabeling, anyway?)
-Eric
Online you mean when mounted ?
Yep - I'm just not sure who would ever want to do that.
Aren't labels primarly used for mounting, during the mount process?

Well if you want to change the label of your root filesystem, how else would you go about that? If Btrfs did not support this, then only while booted from a rescue LiveCD? That's quite a bit more involved and not even always feasible in case of remote machines with no IPMI or similar management.

Extfs supports online change of the volume label as well, albeit probably not via a sysfs file, but with the tune2fs utility:

tune2fs -L <name> /dev/blockdevice

Signature · 2 lines

With respect,
Roman

Eric Sandeen

2014-05-22 16:06 UTC #13 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 18 lines · Roman Mamedov

Quoted 9 lines · On Wed, 21 May 2014 21:14:07 -0500 Eric Sandeen <[email protected]> wrote:
Quoted 5 lines
Quoted 3 lines
(Random aside: why does btrfs support online fs relabeling, anyway?)
-Eric
Online you mean when mounted ?
Yep - I'm just not sure who would ever want to do that.
Aren't labels primarly used for mounting, during the mount process?
Well if you want to change the label of your root filesystem, how else would you go about that? If Btrfs did not support this, then only while booted from a rescue LiveCD? That's quite a bit more involved and not even always feasible in case of remote machines with no IPMI or similar management.

Quoted 4 lines

Extfs supports online change of the volume label as well, albeit probably not via a sysfs file, but with the tune2fs utility:
tune2fs -L <name> /dev/blockdevice

Yeah, that just writes directly to the block device. :) But ok, I guess relabeling root is a fair case.

- -Eric

-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.17 (Darwin)
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTfiCAAAoJECCuFpLhPd7gAHoP/AlFeFJ8Om0wKxcnuolG7o4G 9AHRu4Jto8JTHWToOwmtJAcWQFS+fJtUF009cl118eb+vUOoWFdTDK3H8UiC6uwM /OcFaeDRJOg4JEA8OBaXihCoG1JOPU2ygCezyg59mbHV6SFIYy4kDgqzfFm6qcaC 8vqenmPkGhYiIadvPeDYctIPGS6DquLjxKk/xI52gLwP6so2eJ9nH4jmO7K1gF6n dRa7zfCDpcMq5AqiJW5XZWBfmuNH5etGgJW8J4JpuOSzYPECwuHzcXP3LZny/xUP t15LxzUR+0SZg04zJYXY5YoytZaz6mfk0jOksggeY9GiDenlWUZhDi/QexIhc2ZL ZuHt8WDWDtIqt4D84GE/9QlI5tVU+F8FkJBnPSS+ybP8YSShNWmIdKLVtn+4g8/x YVC6PnUyYvLj/1ZSJTGUQtJZkSjsfkRISk3ygVz1NJQG75euRL2gxmZkn11497rU rc4a+yZQ3Gxe0cOn1J8RzN9dDQ3iIQnM7Fn25vFdn/uN1dPuQuDP3zy9MG4WJD8e 3xX1HNiXePgZGN5bhaCLXqAER6E4Xigd5mcdjuXZvmkexEkTRn3xOcXuoDhU8v2i Oloroh09tpMmzw4sNyaeqw11oIkBcUEvY/7ggqYEtZcOAtnFuyot9y5LUIgNmtBM Sas6/mxzXeTPCApF1o1a
=sUri
-----END PGP SIGNATURE-----

David Sterba

2014-05-20 16:33 UTC #14 lore raw

In reply to Anand Jain

Quoted 22 lines · Anand Jain

From: Anand Jain <[email protected]>
generally if you use
echo "test" > /sys/fs/btrfs/<fsid>/label
it would introduce return char at the end and it can not be part of the label. The correct command is
echo -n "test" > /sys/fs/btrfs/<fsid>/label
This patch will check for this user error
Signed-off-by Anand Jain <[email protected]>
---
v2: accepts review comments. Thanks Eric and Roman
 fs/btrfs/sysfs.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)
fs/btrfs/sysfs.c +0 -0
index c5eb214..ca63fcd 100644

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

Quoted 8 lines

fs/btrfs/sysfs.c +3 -1

@@ -373,22 +373,36 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root = fs_info->fs_root;
 	int ret;
+	char *label;
+	char *pos;
 
-	if (len >= BTRFS_LABEL_SIZE) {
+	label = kzalloc(len, GFP_NOFS);

You can avoid allocating the buffer entirely:

- search for '\n', if found, use only that amount of bytes
- check for maximum size, copy to label if ok

Eric Sandeen

2014-05-20 16:41 UTC #15 lore raw

Quoted 29 lines · David Sterba

Quoted 28 lines · Anand Jain
From: Anand Jain <[email protected]>
generally if you use
echo "test" > /sys/fs/btrfs/<fsid>/label
it would introduce return char at the end and it can not be part of the label. The correct command is
echo -n "test" > /sys/fs/btrfs/<fsid>/label
This patch will check for this user error
Signed-off-by Anand Jain <[email protected]>
---
v2: accepts review comments. Thanks Eric and Roman
 fs/btrfs/sysfs.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)
fs/btrfs/sysfs.c +2 -0
index c5eb214..ca63fcd 100644
@@ -373,22 +373,36 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root = fs_info->fs_root;
 	int ret;
+	char *label;
+	char *pos;
 

Quoted 7 lines

fs/btrfs/sysfs.c static ssize_t btrfs_label_store(struct kobject *kobj, · line 379
Quoted 2 lines
fs/btrfs/sysfs.c +1 -1
-	if (len >= BTRFS_LABEL_SIZE) {
+	label = kzalloc(len, GFP_NOFS);
You can avoid allocating the buffer entirely:
- search for '\n', if found, use only that amount of bytes
- check for maximum size, copy to label if ok

that's probably better than my strstrip idea, which requires writable memory. Odds of finding \t are slim. ;)

-Eric

Anand Jain

author

2014-05-22 10:44 UTC #16 lore raw

In reply to David Sterba

Quoted 29 lines · David Sterba

Quoted 28 lines · Anand Jain
From: Anand Jain <[email protected]>
generally if you use
echo "test" > /sys/fs/btrfs/<fsid>/label
it would introduce return char at the end and it can not be part of the label. The correct command is
echo -n "test" > /sys/fs/btrfs/<fsid>/label
This patch will check for this user error
Signed-off-by Anand Jain <[email protected]>
---
v2: accepts review comments. Thanks Eric and Roman
  fs/btrfs/sysfs.c |   20 +++++++++++++++++---
  1 files changed, 17 insertions(+), 3 deletions(-)
fs/btrfs/sysfs.c +2 -0
index c5eb214..ca63fcd 100644
@@ -373,22 +373,36 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
  	struct btrfs_trans_handle *trans;
  	struct btrfs_root *root = fs_info->fs_root;
  	int ret;
+	char *label;
+	char *pos;
 

Quoted 7 lines

fs/btrfs/sysfs.c static ssize_t btrfs_label_store(struct kobject *kobj, · line 379
Quoted 2 lines
fs/btrfs/sysfs.c +1 -1
-	if (len >= BTRFS_LABEL_SIZE) {
+	label = kzalloc(len, GFP_NOFS);
You can avoid allocating the buffer entirely:
- search for '\n', if found, use only that amount of bytes
- check for maximum size, copy to label if ok

Thats too good.
Thanks.

Anand Jain

author

2014-05-22 10:38 UTC #17 lore raw

PATCH v3 1/2 btrfs: label should not contain return char

In reply to Anand Jain

From: Anand Jain <[email protected]>

This patch will check for this user error

Signed-off-by Anand Jain <[email protected]>

---
v3: accepts review comments. Thanks David and Eric again
v2: accepts review comments. Thanks Eric and Roman

 fs/btrfs/sysfs.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

fs/btrfs/sysfs.c +10 -2

index c5eb214..159df4f 100644
@@ -373,8 +373,15 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root = fs_info->fs_root;
 	int ret;
+	size_t p_len;
 
-	if (len >= BTRFS_LABEL_SIZE) {
+	/*
+	 * p_len is the len until the first occurrence of either
+	 * '\n' or '\0'
+	 */
+	p_len = strcspn(buf, "\n");
+
+	if (p_len >= BTRFS_LABEL_SIZE) {
 		pr_err("BTRFS: unable to set label with more than %d bytes\n",
 		       BTRFS_LABEL_SIZE - 1);
 		return -EINVAL;

@@ -385,7 +392,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 		return PTR_ERR(trans);
 
 	spin_lock(&root->fs_info->super_lock);
-	strcpy(fs_info->super_copy->label, buf);
+	strncpy(fs_info->super_copy->label, buf, p_len);
+	memset(fs_info->super_copy->label + p_len, '\0', 1);
 	spin_unlock(&root->fs_info->super_lock);
 	ret = btrfs_commit_transaction(trans, root);

Signature · 1 line

1.7.1

Anand Jain

author

2014-05-22 10:38 UTC #18 lore raw

PATCH v3 2/2 btrfs: usage error should not be logged into system log

From: Anand Jain <[email protected]>

Signed-off-by Anand Jain <[email protected]>

Reviewed-by David Sterba <[email protected]>

---
v3: rebase again
v2: rebase

 fs/btrfs/sysfs.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

fs/btrfs/sysfs.c +1 -4

index 159df4f..c81b499 100644
@@ -381,11 +381,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	 */
 	p_len = strcspn(buf, "\n");
 
-	if (p_len >= BTRFS_LABEL_SIZE) {
-		pr_err("BTRFS: unable to set label with more than %d bytes\n",
-		       BTRFS_LABEL_SIZE - 1);
+	if (p_len >= BTRFS_LABEL_SIZE)
 		return -EINVAL;
-	}
 
 	trans = btrfs_start_transaction(root, 0);
 	if (IS_ERR(trans))

Signature · 1 line

1.7.1

Koen Kooi

2014-05-22 11:21 UTC #19 lore raw

Re: [PATCH 2/2 v3] btrfs: usage error should not be logged into system log

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Anand Jain schreef op 22-05-14 12:41:

Quoted 7 lines

From: Anand Jain <[email protected]>
I have an opinion that system logs /var/log/messages are valuable info to investigate the real system issues at the data center. People handling data center issues do spend a lot time and efforts analyzing messages files. Having usage error logged into /var/log/messages is something we should avoid.

Do you mean 'syslog' when you say '/var/log/messages'? There's no /var/log/messages on my machines..

regards,

Koen

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Darwin)
Comment: GPGTools - http://gpgtools.org

iD8DBQFTfd2kMkyGM64RGpERAsd6AKCZxfhjjtYWUZLJwS0NnghuCb9lBQCfYye2 L3z3JmZqj9TTb+355MMB6w8=
=SfnW
-----END PGP SIGNATURE-----

Anand Jain

author

2014-05-23 02:38 UTC #20 lore raw

Re: [PATCH 2/2 v3] btrfs: usage error should not be logged into system log

Quoted 6 lines · Koen Kooi

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Anand Jain schreef op 22-05-14 12:41:
Quoted 2 lines
From: Anand Jain <[email protected]>

Quoted 8 lines

Quoted 5 lines
I have an opinion that system logs /var/log/messages are valuable info to investigate the real system issues at the data center. People handling data center issues do spend a lot time and efforts analyzing messages files. Having usage error logged into /var/log/messages is something we should avoid.
Do you mean 'syslog' when you say '/var/log/messages'? There's no /var/log/messages on my machines..

yes indeed, depending on whats configured.
sorry to confuse you. Will update the commit
Thanks, Anand

Quoted 3 lines

regards,
Koen

David Sterba

2014-05-22 11:41 UTC #21 lore raw

In reply to Anand Jain

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

Quoted 7 lines · Anand Jain

fs/btrfs/sysfs.c +2 -1

@@ -385,7 +392,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 		return PTR_ERR(trans);
 
 	spin_lock(&root->fs_info->super_lock);
-	strcpy(fs_info->super_copy->label, buf);
+	strncpy(fs_info->super_copy->label, buf, p_len);
+	memset(fs_info->super_copy->label + p_len, '\0', 1);

This looks strange, memset of length 1?

Anyway, I think the label buffer should be zeroed at the empty space, so the idea is

	memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE):
	memcpy(fs_info->super_copy->label, buf, p_len);

Not super efficient, but works.

Quoted 2 lines

fs/btrfs/sysfs.c +0 -0

 	spin_unlock(&root->fs_info->super_lock);
 	ret = btrfs_commit_transaction(trans, root);

Anand Jain

author

2014-05-23 02:47 UTC #22 lore raw

PATCH v4 1/2 btrfs: label should not contain return char

In reply to Anand Jain

From: Anand Jain <[email protected]>

This patch will check for this user error

Signed-off-by Anand Jain <[email protected]>

---
v4: used memcpy and memset. Thanks David again
v3: accepts review comments. Thanks David and Eric again
v2: accepts review comments. Thanks Eric and Roman

 fs/btrfs/sysfs.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

fs/btrfs/sysfs.c +10 -2

index c5eb214..e12e2be 100644
@@ -373,8 +373,15 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root = fs_info->fs_root;
 	int ret;
+	size_t p_len;
 
-	if (len >= BTRFS_LABEL_SIZE) {
+	/*
+	 * p_len is the len until the first occurrence of either
+	 * '\n' or '\0'
+	 */
+	p_len = strcspn(buf, "\n");
+
+	if (p_len >= BTRFS_LABEL_SIZE) {
 		pr_err("BTRFS: unable to set label with more than %d bytes\n",
 		       BTRFS_LABEL_SIZE - 1);
 		return -EINVAL;

@@ -385,7 +392,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 		return PTR_ERR(trans);
 
 	spin_lock(&root->fs_info->super_lock);
-	strcpy(fs_info->super_copy->label, buf);
+	memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE);
+	memcpy(fs_info->super_copy->label, buf, p_len);
 	spin_unlock(&root->fs_info->super_lock);
 	ret = btrfs_commit_transaction(trans, root);

Signature · 1 line

1.7.1

Anand Jain

author

2014-05-23 02:47 UTC #23 lore raw

PATCH v4 2/2 btrfs: usage error should not be logged into system log

From: Anand Jain <[email protected]>

I have an opinion that system logs under /var/log/ are valuable info to investigate the real system issues at the data center. People handling data center issues do spend a lot time and efforts analyzing messages files. Having usage error logged into system log is something we should avoid.

Signed-off-by Anand Jain <[email protected]>

Reviewed-by David Sterba <[email protected]>

---
v4: commit reword. Thanks Koen
v3: rebase again
v2: rebase

 fs/btrfs/sysfs.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

fs/btrfs/sysfs.c +1 -4

index e12e2be..522d023 100644
@@ -381,11 +381,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	 */
 	p_len = strcspn(buf, "\n");
 
-	if (p_len >= BTRFS_LABEL_SIZE) {
-		pr_err("BTRFS: unable to set label with more than %d bytes\n",
-		       BTRFS_LABEL_SIZE - 1);
+	if (p_len >= BTRFS_LABEL_SIZE)
 		return -EINVAL;
-	}
 
 	trans = btrfs_start_transaction(root, 0);
 	if (IS_ERR(trans))

Signature · 1 line

1.7.1

David Sterba

Reviewed-by on v4 1/2

2014-05-26 17:41 UTC #24 lore raw

In reply to Anand Jain

Quoted 11 lines · Anand Jain

From: Anand Jain <[email protected]>
generally if you use
echo "test" > /sys/fs/btrfs/<fsid>/label
it would introduce return char at the end and it can not be part of the label. The correct command is
echo -n "test" > /sys/fs/btrfs/<fsid>/label
This patch will check for this user error
Signed-off-by Anand Jain <[email protected]>

Reviewed-by David Sterba <[email protected]>

Thanks.

Satoru Takeuchi

2014-07-01 05:23 UTC #25 lore raw

PATCH v5 1/1 btrfs: label should not contain return char

In reply to Anand Jain

Although Anand once sent the following two patches,

- [PATCH 1/2 v4] btrfs: label should not contain return char
- [PATCH 2/2 v4] btrfs: usage error should not be logged into system log

only the latter patch was merged to mason/for-linus and 3.16-rc3 as 402a0f4 (by accident?). It results in that the former patch can't be cleanly applied to 3.16-rc3.

I fixed this problem, wrote a reproducer, and tested it.

Test Result:
3.16-rc3 w/o this patch: fail
3.16-rc3 w/ this patch: pass

Subject: [PATCH v5] btrfs: label should not contain return char

From: Anand Jain <[email protected]>

This patch will check for this user error

reproducer.sh:
===============================================================================
#!/bin/bash

TEST_DEV=/dev/vdb
TEST_DIR=/home/sat/mnt

umount /home/sat/mnt

mkfs.btrfs -f $TEST_DEV
UUID=$(btrfs fi show $TEST_DEV | head -1 | sed -e 's/.*uuid: $[-0-9a-z]*$$/\1/') mount $TEST_DEV $TEST_DIR
LABELFILE=/sys/fs/btrfs/$UUID/label

echo testlabel >$LABELFILE
LINES="$(cat $LABELFILE | wc -l | awk '{print $1}')"

RET=1
if [ $LINES -eq 1 ] ; then
echo '[PASS] Trailing \n is removed correctly.' >&2 RET=0
else
echo '[FAIL] Trailing \n still exists.' >&2
fi

exit $RET
===============================================================================

Signed-off-by Anand Jain <[email protected]>

Signed-off-by Satoru Takeuchi <[email protected]>

Reviewed-by Satoru Takeuchi <[email protected]>

Tested-by Satoru Takeuchi <[email protected]>

---
v5: tweak to be able to apply on the top of 402a0f4.
add test program.
v4: used memcpy and memset. Thanks David again
v3: accepts review comments. Thanks David and Eric again
v2: accepts review comments. Thanks Eric and Roman

---
 fs/btrfs/sysfs.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

fs/btrfs/sysfs.c +10 -2

index df39458..dcae61a 100644
@@ -374,8 +374,15 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root = fs_info->fs_root;
 	int ret;
+	size_t p_len;
 
-	if (len >= BTRFS_LABEL_SIZE)
+	/*
+	 * p_len is the len until the first occurrence of either
+	 * '\n' or '\0'
+	 */
+	p_len = strcspn(buf, "\n");
+
+	if (p_len >= BTRFS_LABEL_SIZE)
 		return -EINVAL;
 
 	trans = btrfs_start_transaction(root, 0);

@@ -383,7 +390,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 		return PTR_ERR(trans);
 
 	spin_lock(&root->fs_info->super_lock);
-	strcpy(fs_info->super_copy->label, buf);
+	memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE);
+	memcpy(fs_info->super_copy->label, buf, p_len);
 	spin_unlock(&root->fs_info->super_lock);
 	ret = btrfs_commit_transaction(trans, root);

Signature · 1 line

1.9.3

Wang Shilong

2014-07-01 06:50 UTC #26 lore raw

Hi Satoru and all,

I think there maybe a leftover issue.
That is if we don't set label, in default it will output a blank line.

Steps to reproduce:

# mkfs.btrfs -f /dev/sdb
# mount /dev/sdb
# cat /sys/fs/btrfs/<uuid>/label -->an extra line will be outputed.

This is because in btrfs_label_show(), we did something like this directly:

return snprintf(buf, PAGE_SIZE, "%s\n", fs_info->super_copy->label);

Maybe we can have a check whether label is NULL before we output? otherwise,the
extra blank line is outputed, IMO this is not so nice thing!

Thanks,
Wang

Quoted 103 lines · Satoru Takeuchi

Although Anand once sent the following two patches,
- [PATCH 1/2 v4] btrfs: label should not contain return char
- [PATCH 2/2 v4] btrfs: usage error should not be logged into system log
only the latter patch was merged to mason/for-linus and 3.16-rc3 as 402a0f4 (by accident?). It results in that the former patch can't be cleanly applied to 3.16-rc3.
I fixed this problem, wrote a reproducer, and tested it.
Test Result:
3.16-rc3 w/o this patch: fail
3.16-rc3 w/ this patch: pass
Subject: [PATCH v5] btrfs: label should not contain return char
From: Anand Jain <[email protected]>
generally if you use
echo "test" > /sys/fs/btrfs/<fsid>/label
it would introduce return char at the end and it can not be part of the label. The correct command is
echo -n "test" > /sys/fs/btrfs/<fsid>/label
This patch will check for this user error
reproducer.sh:
===============================================================================
#!/bin/bash
TEST_DEV=/dev/vdb
TEST_DIR=/home/sat/mnt
umount /home/sat/mnt
mkfs.btrfs -f $TEST_DEV
UUID=$(btrfs fi show $TEST_DEV | head -1 | sed -e 's/.*uuid: $[-0-9a-z]*$$/\1/') mount $TEST_DEV $TEST_DIR
LABELFILE=/sys/fs/btrfs/$UUID/label
echo testlabel >$LABELFILE
LINES="$(cat $LABELFILE | wc -l | awk '{print $1}')"
RET=1
if [ $LINES -eq 1 ] ; then
echo '[PASS] Trailing \n is removed correctly.' >&2 RET=0
else
echo '[FAIL] Trailing \n still exists.' >&2
fi
exit $RET
===============================================================================
Signed-off-by Anand Jain <[email protected]>
Signed-off-by Satoru Takeuchi <[email protected]>
Reviewed-by Satoru Takeuchi <[email protected]>
Tested-by Satoru Takeuchi <[email protected]>
---
v5: tweak to be able to apply on the top of 402a0f4.
add test program.
v4: used memcpy and memset. Thanks David again
v3: accepts review comments. Thanks David and Eric again
v2: accepts review comments. Thanks Eric and Roman
---
 fs/btrfs/sysfs.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
fs/btrfs/sysfs.c +10 -2
index df39458..dcae61a 100644
@@ -374,8 +374,15 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root = fs_info->fs_root;
 	int ret;
+	size_t p_len;
 
-	if (len >= BTRFS_LABEL_SIZE)
+	/*
+	 * p_len is the len until the first occurrence of either
+	 * '\n' or '\0'
+	 */
+	p_len = strcspn(buf, "\n");
+
+	if (p_len >= BTRFS_LABEL_SIZE)
 		return -EINVAL;
 
 	trans = btrfs_start_transaction(root, 0);
@@ -383,7 +390,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 		return PTR_ERR(trans);
 
 	spin_lock(&root->fs_info->super_lock);
-	strcpy(fs_info->super_copy->label, buf);
+	memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE);
+	memcpy(fs_info->super_copy->label, buf, p_len);
 	spin_unlock(&root->fs_info->super_lock);
 	ret = btrfs_commit_transaction(trans, root);
 

Satoru Takeuchi

2014-07-01 08:00 UTC #27 lore raw

PATCH v6 1/1 btrfs: label should not contain return char

Hi Wang,

Quoted 11 lines · Wang Shilong

Hi Satoru and all,
I think there maybe a leftover issue.
That is if we don't set label, in default it will output a blank line.
Steps to reproduce:
# mkfs.btrfs -f /dev/sdb
# mount /dev/sdb
# cat /sys/fs/btrfs/<uuid>/label -->an extra line will be outputed.

Quoted 7 lines

This is because in btrfs_label_show(), we did something like this directly:
return snprintf(buf, PAGE_SIZE, "%s\n", fs_info->super_copy->label);
Maybe we can have a check whether label is NULL before we output? otherwise,the
extra blank line is outputed, IMO this is not so nice thing!

OK, how about it is? I also add a test for empty-label case.

From: Anand Jain <[email protected]>

This patch will check for this user error

reproducer.sh:
===============================================================================
#!/bin/bash

TEST_DEV=/dev/vdb
TEST_DIR=/home/sat/mnt

umount /home/sat/mnt

mkfs.btrfs -f $TEST_DEV
UUID=$(btrfs fi show $TEST_DEV | head -1 | sed -e 's/.*uuid: $[-0-9a-z]*$$/\1/') mount $TEST_DEV $TEST_DIR
LABELFILE=/sys/fs/btrfs/$UUID/label

echo "Test for empty label..." >&2 LINES="$(cat $LABELFILE | wc -l | awk '{print $1}')" RET=0

if [ $LINES -eq 0 ] ; then
echo '[PASS] Trailing \n is removed correctly.' >&2
else
echo '[FAIL] Trailing \n still exists.' >&2 RET=1
fi

echo "Test for non-empty label..." >&2

echo testlabel >$LABELFILE
LINES="$(cat $LABELFILE | wc -l | awk '{print $1}')"

if [ $LINES -eq 1 ] ; then
echo '[PASS] Trailing \n is removed correctly.' >&2
else
echo '[FAIL] Trailing \n still exists.' >&2 RET=1
fi

exit $RET
===============================================================================

Signed-off-by Anand Jain <[email protected]>

Signed-off-by Satoru Takeuchi <[email protected]>

Reviewed-by Satoru Takeuchi <[email protected]>

Tested-by Satoru Takeuchi <[email protected]>

---
v6: fix empty-label case
v5: tweak to be able to apply on the top of 402a0f4.
add test program.
v4: used memcpy and memset. Thanks David again
v3: accepts review comments. Thanks David and Eric again
v2: accepts review comments. Thanks Eric and Roman

---
 fs/btrfs/sysfs.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

fs/btrfs/sysfs.c +12 -3

index df39458..68d1f63 100644
@@ -363,7 +363,8 @@ static ssize_t btrfs_label_show(struct kobject *kobj,
 				struct kobj_attribute *a, char *buf)
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
-	return snprintf(buf, PAGE_SIZE, "%s\n", fs_info->super_copy->label);
+	char *label = fs_info->super_copy->label;
+	return snprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);
 }
 
 static ssize_t btrfs_label_store(struct kobject *kobj,

@@ -374,8 +375,15 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root = fs_info->fs_root;
 	int ret;
+	size_t p_len;
 
-	if (len >= BTRFS_LABEL_SIZE)
+	/*
+	 * p_len is the len until the first occurrence of either
+	 * '\n' or '\0'
+	 */
+	p_len = strcspn(buf, "\n");
+
+	if (p_len >= BTRFS_LABEL_SIZE)
 		return -EINVAL;
 
 	trans = btrfs_start_transaction(root, 0);

@@ -383,7 +391,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 		return PTR_ERR(trans);
 
 	spin_lock(&root->fs_info->super_lock);
-	strcpy(fs_info->super_copy->label, buf);
+	memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE);
+	memcpy(fs_info->super_copy->label, buf, p_len);
 	spin_unlock(&root->fs_info->super_lock);
 	ret = btrfs_commit_transaction(trans, root);

Signature · 1 line

1.9.3

Wang Shilong

Reviewed-by on v6 1/1

2014-07-01 08:33 UTC #28 lore raw

Hi Satoru,

Quoted 72 lines · Satoru Takeuchi

Hi Wang,
Quoted 11 lines · Wang Shilong
Hi Satoru and all,
I think there maybe a leftover issue.
That is if we don't set label, in default it will output a blank line.
Steps to reproduce:
# mkfs.btrfs -f /dev/sdb
# mount /dev/sdb
# cat /sys/fs/btrfs/<uuid>/label -->an extra line will be outputed.
Quoted 7 lines
This is because in btrfs_label_show(), we did something like this directly:
return snprintf(buf, PAGE_SIZE, "%s\n", fs_info->super_copy->label);
Maybe we can have a check whether label is NULL before we output? otherwise,the
extra blank line is outputed, IMO this is not so nice thing!
OK, how about it is? I also add a test for empty-label case.
From: Anand Jain <[email protected]>
generally if you use
echo "test" > /sys/fs/btrfs/<fsid>/label
it would introduce return char at the end and it can not be part of the label. The correct command is
echo -n "test" > /sys/fs/btrfs/<fsid>/label
This patch will check for this user error
reproducer.sh:
===============================================================================
#!/bin/bash
TEST_DEV=/dev/vdb
TEST_DIR=/home/sat/mnt
umount /home/sat/mnt
mkfs.btrfs -f $TEST_DEV
UUID=$(btrfs fi show $TEST_DEV | head -1 | sed -e 's/.*uuid: $[-0-9a-z]*$$/\1/') mount $TEST_DEV $TEST_DIR
LABELFILE=/sys/fs/btrfs/$UUID/label
echo "Test for empty label..." >&2 LINES="$(cat $LABELFILE | wc -l | awk '{print $1}')" RET=0
if [ $LINES -eq 0 ] ; then
echo '[PASS] Trailing \n is removed correctly.' >&2
else
echo '[FAIL] Trailing \n still exists.' >&2 RET=1
fi
echo "Test for non-empty label..." >&2
echo testlabel >$LABELFILE
LINES="$(cat $LABELFILE | wc -l | awk '{print $1}')"
if [ $LINES -eq 1 ] ; then
echo '[PASS] Trailing \n is removed correctly.' >&2
else
echo '[FAIL] Trailing \n still exists.' >&2 RET=1
fi

Quoted 7 lines

exit $RET
===============================================================================
Signed-off-by Anand Jain <[email protected]>
Signed-off-by Satoru Takeuchi <[email protected]>
Reviewed-by Satoru Takeuchi <[email protected]>
Tested-by Satoru Takeuchi <[email protected]>

Thanks for pushing this patch, Now it Looks good to me.

Reviewed-by Wang Shilong <[email protected]>

Quoted 52 lines

---
 fs/btrfs/sysfs.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

fs/btrfs/sysfs.c +12 -3

index df39458..68d1f63 100644
@@ -363,7 +363,8 @@ static ssize_t btrfs_label_show(struct kobject *kobj,
 				struct kobj_attribute *a, char *buf)
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
-	return snprintf(buf, PAGE_SIZE, "%s\n", fs_info->super_copy->label);
+	char *label = fs_info->super_copy->label;
+	return snprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);
 }
 
 static ssize_t btrfs_label_store(struct kobject *kobj,

@@ -374,8 +375,15 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root = fs_info->fs_root;
 	int ret;
+	size_t p_len;
 
-	if (len >= BTRFS_LABEL_SIZE)
+	/*
+	 * p_len is the len until the first occurrence of either
+	 * '\n' or '\0'
+	 */
+	p_len = strcspn(buf, "\n");
+
+	if (p_len >= BTRFS_LABEL_SIZE)
 		return -EINVAL;
 
 	trans = btrfs_start_transaction(root, 0);

@@ -383,7 +391,8 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 		return PTR_ERR(trans);
 
 	spin_lock(&root->fs_info->super_lock);
-	strcpy(fs_info->super_copy->label, buf);
+	memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE);
+	memcpy(fs_info->super_copy->label, buf, p_len);
 	spin_unlock(&root->fs_info->super_lock);
 	ret = btrfs_commit_transaction(trans, root);

David Sterba

Reviewed-by on v6 1/1

2014-07-01 15:05 UTC #29 lore raw

In reply to Satoru Takeuchi

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

Quoted 9 lines · Satoru Takeuchi

fs/btrfs/sysfs.c +2 -1

@@ -363,7 +363,8 @@ static ssize_t btrfs_label_show(struct kobject *kobj,
 				struct kobj_attribute *a, char *buf)
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
-	return snprintf(buf, PAGE_SIZE, "%s\n", fs_info->super_copy->label);
+	char *label = fs_info->super_copy->label;
+	return snprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);

I'd prefer "" instead of the "%s" branch, but does not matter that much,

Reviewed-by David Sterba <[email protected]>