Skip to contentSkip to conversation

NFSD: Ignore and warn once for more fslocs or more uuids

PATCH In review Kinglong Mee 2 participants 6 messages May 23, 2014 – May 27, 2014

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Kinglong Mee

author
#0 lore raw

PATCH 4/4 NFSD: Ignore and warn once for more fslocs or more uuids

If mountd writes more fslocs or more uuids, just ignore and warn once.

Signed-off-by Kinglong Mee <[email protected]>
---
 fs/nfsd/export.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
fs/nfsd/export.c +12 -0
index 90d37b6..1d56ae3 100644
@@ -394,6 +394,12 @@ fsloc_parse(char **mesg, char *buf, struct nfsd4_fs_locations *fsloc)
 	int len;
 	int migrated, i, err;
 
+	/* utils writes more fslocs than one */
+	if (fsloc->locations) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
 	/* listsize */
 	err = get_uint(mesg, &fsloc->locations_count);
 	if (err)

@@ -485,6 +491,12 @@ uuid_parse(char **mesg, char *buf, unsigned char **puuid)
 {
 	int len;
 
+	/* utils writes more uuid than one */
+	if (*puuid) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
 	/* expect a 16 byte uuid encoded as \xXXXX... */
 	len = qword_get(mesg, buf, PAGE_SIZE);
 	if (len != EX_UUID_LEN)
Signature · 1 line
1.9.0
#1 lore raw
Quoted 1 line · Kinglong Mee

If mountd writes more fslocs or more uuids, just ignore and warn once.

So the current behavior is a leak?

Have you observed any version of mountd actually writing more than one of these?

If not, I'd prefer to return an error.

--b.

Quoted 39 lines
Signed-off-by Kinglong Mee <[email protected]>
---
 fs/nfsd/export.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
fs/nfsd/export.c +12 -0
index 90d37b6..1d56ae3 100644
@@ -394,6 +394,12 @@ fsloc_parse(char **mesg, char *buf, struct nfsd4_fs_locations *fsloc)
 	int len;
 	int migrated, i, err;
 
+	/* utils writes more fslocs than one */
+	if (fsloc->locations) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
 	/* listsize */
 	err = get_uint(mesg, &fsloc->locations_count);
 	if (err)

@@ -485,6 +491,12 @@ uuid_parse(char **mesg, char *buf, unsigned char **puuid)
 {
 	int len;
 
+	/* utils writes more uuid than one */
+	if (*puuid) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
 	/* expect a 16 byte uuid encoded as \xXXXX... */
 	len = qword_get(mesg, buf, PAGE_SIZE);
 	if (len != EX_UUID_LEN)
Signature · 1 line
1.9.0
author
#2 lore raw
Quoted 4 lines · J. Bruce Fields
Quoted 1 line · Kinglong Mee

If mountd writes more fslocs or more uuids, just ignore and warn once.

So the current behavior is a leak?

Yes, there is a memory leak for more fslocs or more uuids.

Quoted 3 lines

Have you observed any version of mountd actually writing more than one of these?

Sorry, i don't checkout it.
I just review the codes, and rewrit nfs-utils to writing more fslocs or uuids.

Quoted 2 lines

If not, I'd prefer to return an error.

You mean return -EINVAL instead of ignoring it with return 0?

Although, i miss checking the more "secinfo" in secinfo_parse() right now, for no memory leak. I will adds it after decides how to fix above.

thanks,
Kinglong Mee

Quoted 43 lines

--b.

Quoted 39 lines
Signed-off-by Kinglong Mee <[email protected]>
---
 fs/nfsd/export.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
fs/nfsd/export.c +12 -0
index 90d37b6..1d56ae3 100644
@@ -394,6 +394,12 @@ fsloc_parse(char **mesg, char *buf, struct nfsd4_fs_locations *fsloc)
 	int len;
 	int migrated, i, err;
 
+	/* utils writes more fslocs than one */
+	if (fsloc->locations) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
 	/* listsize */
 	err = get_uint(mesg, &fsloc->locations_count);
 	if (err)

@@ -485,6 +491,12 @@ uuid_parse(char **mesg, char *buf, unsigned char **puuid)
 {
 	int len;
 
+	/* utils writes more uuid than one */
+	if (*puuid) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
 	/* expect a 16 byte uuid encoded as \xXXXX... */
 	len = qword_get(mesg, buf, PAGE_SIZE);
 	if (len != EX_UUID_LEN)
Signature · 1 line
1.9.0
#3 lore raw
Quoted 12 lines · Kinglong Mee
Quoted 4 lines · J. Bruce Fields
Quoted 1 line · Kinglong Mee

If mountd writes more fslocs or more uuids, just ignore and warn once.

So the current behavior is a leak?

Yes, there is a memory leak for more fslocs or more uuids.

Quoted 3 lines

Have you observed any version of mountd actually writing more than one of these?

Quoted 7 lines

Sorry, i don't checkout it.
I just review the codes, and rewrit nfs-utils to writing more fslocs or uuids.

Quoted 2 lines

If not, I'd prefer to return an error.

You mean return -EINVAL instead of ignoring it with return 0?

Right. If somebody introduces weird mountd behavior I'd rather they see it fail right away. And also I think it's a little easier to extend these calls later if we error out on invalid input.

Also, remove the WARN_ON_ONCE. We usually reserve BUG or WARN for cases where we believe the kernel itself has a bug, and avoid them in cases where they could be triggered by userland. (Admittedly mountd is very special userland, but still I'd rather it not be able to trigger these.)

--b.

Quoted 50 lines

Although, i miss checking the more "secinfo" in secinfo_parse() right now, for no memory leak. I will adds it after decides how to fix above.

thanks,
Kinglong Mee

Quoted 43 lines

--b.

Quoted 39 lines
Signed-off-by Kinglong Mee <[email protected]>
---
 fs/nfsd/export.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
fs/nfsd/export.c +12 -0
index 90d37b6..1d56ae3 100644
@@ -394,6 +394,12 @@ fsloc_parse(char **mesg, char *buf, struct nfsd4_fs_locations *fsloc)
 	int len;
 	int migrated, i, err;
 
+	/* utils writes more fslocs than one */
+	if (fsloc->locations) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
 	/* listsize */
 	err = get_uint(mesg, &fsloc->locations_count);
 	if (err)

@@ -485,6 +491,12 @@ uuid_parse(char **mesg, char *buf, unsigned char **puuid)
 {
 	int len;
 
+	/* utils writes more uuid than one */
+	if (*puuid) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
 	/* expect a 16 byte uuid encoded as \xXXXX... */
 	len = qword_get(mesg, buf, PAGE_SIZE);
 	if (len != EX_UUID_LEN)
Signature · 1 line
1.9.0
author
#4 lore raw

PATCH v2 1/1 NFSD: Error out when getting more than one fsloc/secinfo/uuid

v2:
remove WARN_ON_ONCE(1) and return -EINVAL instead of ignoring it.

Signed-off-by Kinglong Mee <[email protected]>
---
 fs/nfsd/export.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
fs/nfsd/export.c +12 -0
index 7884051..13b85f9 100644
@@ -389,6 +389,10 @@ fsloc_parse(char **mesg, char *buf, struct nfsd4_fs_locations *fsloc)
 	int len;
 	int migrated, i, err;
 
+	/* more than one fsloc */
+	if (fsloc->locations)
+		return -EINVAL;
+
 	/* listsize */
 	err = get_uint(mesg, &fsloc->locations_count);
 	if (err)

@@ -442,6 +446,10 @@ static int secinfo_parse(char **mesg, char *buf, struct svc_export *exp)
 	u32 listsize;
 	int err;
 
+	/* more than one secinfo */
+	if (exp->ex_nflavors)
+		return -EINVAL;
+
 	err = get_uint(mesg, &listsize);
 	if (err)
 		return err;

@@ -481,6 +489,10 @@ uuid_parse(char **mesg, char *buf, unsigned char **puuid)
 {
 	int len;
 
+	/* more than one uuid */
+	if (*puuid)
+		return -EINVAL;
+
 	/* expect a 16 byte uuid encoded as \xXXXX... */
 	len = qword_get(mesg, buf, PAGE_SIZE);
 	if (len != EX_UUID_LEN)
Signature · 1 line
1.9.3
#5 lore raw

Re: [PATCH v2] NFSD: Error out when getting more than one fsloc/secinfo/uuid

Quoted 2 lines · Kinglong Mee

v2:
remove WARN_ON_ONCE(1) and return -EINVAL instead of ignoring it.

Thanks, applying.--b.

Quoted 46 lines
Signed-off-by Kinglong Mee <[email protected]>
---
 fs/nfsd/export.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
fs/nfsd/export.c +12 -0
index 7884051..13b85f9 100644
@@ -389,6 +389,10 @@ fsloc_parse(char **mesg, char *buf, struct nfsd4_fs_locations *fsloc)
 	int len;
 	int migrated, i, err;
 
+	/* more than one fsloc */
+	if (fsloc->locations)
+		return -EINVAL;
+
 	/* listsize */
 	err = get_uint(mesg, &fsloc->locations_count);
 	if (err)

@@ -442,6 +446,10 @@ static int secinfo_parse(char **mesg, char *buf, struct svc_export *exp)
 	u32 listsize;
 	int err;
 
+	/* more than one secinfo */
+	if (exp->ex_nflavors)
+		return -EINVAL;
+
 	err = get_uint(mesg, &listsize);
 	if (err)
 		return err;

@@ -481,6 +489,10 @@ uuid_parse(char **mesg, char *buf, unsigned char **puuid)
 {
 	int len;
 
+	/* more than one uuid */
+	if (*puuid)
+		return -EINVAL;
+
 	/* expect a 16 byte uuid encoded as \xXXXX... */
 	len = qword_get(mesg, buf, PAGE_SIZE);
 	if (len != EX_UUID_LEN)
Signature · 1 line
1.9.3