Skip to contentSkip to conversation

ss: fix the incorrect value of total UNIX_DIAG_* number

PATCH NAK Shan Wei 3 participants 11 messages Apr 25, 2012

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Shan Wei

author
#0 lore raw

PATCH 1/2 ss: fix the incorrect value of total UNIX_DIAG_* number

From: Shan Wei <[email protected]>

UNIX_DIAG_MAX is included in enum type.
It is equal to the total number of enum element.

But lots of enum MAX value is defined as the max enum element, e.g. INET_DIAG_MAX, XFRMA_MAX. The right fixing way seems to define UNIX_DIAG_MAX as UNIX_DIAG_MEMINFO,
but this way will break other user application.

So, just fix it on user application.

Signed-off-by Shan Wei <[email protected]>
---
 misc/ss.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
misc/ss.c +2 -2
index 4017918..5f70a26 100644
@@ -2011,12 +2011,12 @@ void unix_list_print(struct unixstat *list, struct filter *f)
 static int unix_show_sock(struct nlmsghdr *nlh, struct filter *f)
 {
 	struct unix_diag_msg *r = NLMSG_DATA(nlh);
-	struct rtattr *tb[UNIX_DIAG_MAX+1];
+	struct rtattr *tb[UNIX_DIAG_MAX];
 	char name[128];
 	int peer_ino;
 	int rqlen;
 
-	parse_rtattr(tb, UNIX_DIAG_MAX, (struct rtattr*)(r+1),
+	parse_rtattr(tb, UNIX_DIAG_MAX-1, (struct rtattr*)(r+1),
 		     nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
 
 	if (netid_width)
Signature · 1 line
1.7.1
NAK
#1 lore raw
Quoted 10 lines · On Wed, 25 Apr 2012 15:15:16 +0800 Shan Wei <[email protected]> wrote:

From: Shan Wei <[email protected]>

UNIX_DIAG_MAX is included in enum type.
It is equal to the total number of enum element.

But lots of enum MAX value is defined as the max enum element, e.g. INET_DIAG_MAX, XFRMA_MAX. The right fixing way seems to define UNIX_DIAG_MAX as UNIX_DIAG_MEMINFO,
but this way will break other user application.

So, just fix it on user application.

Nak, we should fix this in the kernel. It is ridiculous to have a convention that is true for one route attribute type but not for all of them.

#2 lore raw

PATCH 1/1 unix_diag: use netlink attribute MAX convention

Use the standard convention to define the number of elements in unix diag attribute. This fixes future problems like the fact the last element (MEMINFO) is not parsed by current iproute2 ss command.

Signed-off-by Stephen Hemminger <[email protected]>
include/linux/unix_diag.h +2 -2
@@ -37,9 +37,9 @@ enum {
 	UNIX_DIAG_ICONS,
 	UNIX_DIAG_RQLEN,
 	UNIX_DIAG_MEMINFO,
-
-	UNIX_DIAG_MAX,
+	__UNIX_DIAG_MAX
 };
+#define UNIX_DIAG_MAX (__UNIX_DIAG_MAX - 1)
 
 struct unix_diag_vfs {
 	__u32	udiag_vfs_ino;
maintainer
#3 lore raw

Re: [PATCH] unix_diag: use netlink attribute MAX convention

From: Stephen Hemminger <[email protected]>
Date: Wed, 25 Apr 2012 10:38:58 -0700

Quoted 5 lines

Use the standard convention to define the number of elements in unix diag attribute. This fixes future problems like the fact the last element (MEMINFO) is not parsed by current iproute2 ss command.

Signed-off-by Stephen Hemminger <[email protected]>

You can't change this, it's already out in the wild, and for 2 releases.

Sorry.

Just accept the 'ss' patch that was posted and you objected to, I'm not breaking userspace just so that you don't have to apply a userland patch you perceive as an unacceptable quirk.

#4 lore raw

Re: [PATCH] unix_diag: use netlink attribute MAX convention

Quoted 9 lines · On Wed, 25 Apr 2012 14:16:58 -0400 (EDT) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Wed, 25 Apr 2012 10:38:58 -0700

Quoted 5 lines

Use the standard convention to define the number of elements in unix diag attribute. This fixes future problems like the fact the last element (MEMINFO) is not parsed by current iproute2 ss command.

Signed-off-by Stephen Hemminger <[email protected]>
Quoted 8 lines

You can't change this, it's already out in the wild, and for 2 releases.

Sorry.

Just accept the 'ss' patch that was posted and you objected to, I'm not breaking userspace just so that you don't have to apply a userland patch you perceive as an unacceptable quirk.

It only changes a broken enum value used only by iproute2. Do you expect the one utility to use it to have a workaround for a broken initial version.

maintainer
#5 lore raw

Re: [PATCH] unix_diag: use netlink attribute MAX convention

From: Stephen Hemminger <[email protected]>
Date: Wed, 25 Apr 2012 11:56:09 -0700

Quoted 2 lines

Do you expect the one utility to use it to have a workaround for a broken initial version.

I'm not taking that risk.

They thought the same exact thing when they did the autofs struct size compat fix, and it turned out to break things.

Stephen I have an awesome suggestion for you if you want to avoid this in the future, review iproute2 patches more aggressively so you can catch things like this earlier. Like, when we can actually still safely change things.

Because currently you let patches rot in patchwork. There's an iproute2 patch in there assigned to you which is 3 months old, that simply isn't how this is supposed to work.

I hate to keep beating a dead horse, but you don't stay on top of patchwork like you should. The object is not to let patches just rot in "Under Review" state for months.

Either you apply them as soon as possible, or you mark them appropriately as "Changes Requested" or "Deferred" so that the submitter makes appropriate fixes you've asked for, or resubmits when it's more appropriate for the change to go in.

"Under Review" doesn't mean, "I'm waiting for a kernel release with the feature". But that's how you use it.

#6 lore raw

Re: [PATCH] unix_diag: use netlink attribute MAX convention

Quoted 8 lines · On Wed, 25 Apr 2012 15:07:16 -0400 (EDT) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Wed, 25 Apr 2012 11:56:09 -0700

Quoted 2 lines

Do you expect the one utility to use it to have a workaround for a broken initial version.

I'm not taking that risk.

Quoted 7 lines

They thought the same exact thing when they did the autofs struct size compat fix, and it turned out to break things.

Stephen I have an awesome suggestion for you if you want to avoid this in the future, review iproute2 patches more aggressively so you can catch things like this earlier. Like, when we can actually still safely change things.

Sorry it was more of cross project issue in this case. The original kernel patch had the problem and was lost in the fog of the other issues like the unix diag implementation not building.

A community works best if multiple people look at the code. Don't think I would have spotted it unless I compared it to other places.

Quoted 7 lines

Because currently you let patches rot in patchwork. There's an iproute2 patch in there assigned to you which is 3 months old, that simply isn't how this is supposed to work.

I hate to keep beating a dead horse, but you don't stay on top of patchwork like you should. The object is not to let patches just rot in "Under Review" state for months.

I keep patches that are for -next in that state.

Quoted 7 lines

Either you apply them as soon as possible, or you mark them appropriately as "Changes Requested" or "Deferred" so that the submitter makes appropriate fixes you've asked for, or resubmits when it's more appropriate for the change to go in.

"Under Review" doesn't mean, "I'm waiting for a kernel release with the feature". But that's how you use it.

Ok. What is the suggested tag for that.

maintainer
#7 lore raw

Re: [PATCH] unix_diag: use netlink attribute MAX convention

From: Stephen Hemminger <[email protected]>
Date: Wed, 25 Apr 2012 12:47:43 -0700

Quoted 8 lines
Quoted 5 lines · On Wed, 25 Apr 2012 15:07:16 -0400 (EDT) David Miller <[email protected]>…

Either you apply them as soon as possible, or you mark them appropriately as "Changes Requested" or "Deferred" so that the submitter makes appropriate fixes you've asked for, or resubmits when it's more appropriate for the change to go in.

Quoted 4 lines
Quoted 2 lines

"Under Review" doesn't mean, "I'm waiting for a kernel release with the feature". But that's how you use it.

Ok. What is the suggested tag for that.

As I said above, "Deferred", with a note sent to the submitter to resubmit the patch at the appropriate time.

#8 lore raw

Re: [PATCH] unix_diag: use netlink attribute MAX convention

Quoted 11 lines · On Wed, 25 Apr 2012 15:57:17 -0400 (EDT) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Wed, 25 Apr 2012 12:47:43 -0700

Quoted 8 lines
Quoted 5 lines · On Wed, 25 Apr 2012 15:07:16 -0400 (EDT) David Miller <[email protected]>…

Either you apply them as soon as possible, or you mark them appropriately as "Changes Requested" or "Deferred" so that the submitter makes appropriate fixes you've asked for, or resubmits when it's more appropriate for the change to go in.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

"Under Review" doesn't mean, "I'm waiting for a kernel release with the feature". But that's how you use it.

Ok. What is the suggested tag for that.

As I said above, "Deferred", with a note sent to the submitter to resubmit the patch at the appropriate time.

That doesn't work, not asking original submitter to resubmit.
I tried a branch, but git doesn't like branches coming and going on remote repos.
Probably better to just have an ephemeral iproute2-next repo on kernel.org.

maintainer
#9 lore raw

Re: [PATCH] unix_diag: use netlink attribute MAX convention

From: Stephen Hemminger <[email protected]>
Date: Wed, 25 Apr 2012 13:09:06 -0700

Quoted 1 line

That doesn't work, not asking original submitter to resubmit.

It works every single time for me.

If they don't resubmit, they don't really care about their work being included, and therefore neither should you.

End of story.

#10 lore raw
In reply to Shan Wei
Quoted 26 lines · On Wed, 25 Apr 2012 15:15:16 +0800 Shan Wei <[email protected]> wrote:

From: Shan Wei <[email protected]>

UNIX_DIAG_MAX is included in enum type.
It is equal to the total number of enum element.

But lots of enum MAX value is defined as the max enum element, e.g. INET_DIAG_MAX, XFRMA_MAX. The right fixing way seems to define UNIX_DIAG_MAX as UNIX_DIAG_MEMINFO,
but this way will break other user application.

So, just fix it on user application.

Signed-off-by Shan Wei <[email protected]>
---
 misc/ss.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
misc/ss.c +0 -1
index 4017918..5f70a26 100644
@@ -2011,12 +2011,12 @@ void unix_list_print(struct unixstat *list, struct filter *f)
 static int unix_show_sock(struct nlmsghdr *nlh, struct filter *f)
 {
 	struct unix_diag_msg *r = NLMSG_DATA(nlh);
-	struct rtattr *tb[UNIX_DIAG_MAX+1];

misc/ss.c void unix_list_print(struct unixstat *list, struct filter *f) · line 2022

Quoted 10 lines
misc/ss.c +2 -1
+	struct rtattr *tb[UNIX_DIAG_MAX];
 	char name[128];
 	int peer_ino;
 	int rqlen;
 
-	parse_rtattr(tb, UNIX_DIAG_MAX, (struct rtattr*)(r+1),
+	parse_rtattr(tb, UNIX_DIAG_MAX-1, (struct rtattr*)(r+1),
 		     nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
 
 	if (netid_width)

Ok, I'll put this in but with a big juicy comment so that nobody else is surprised by the fact we got it wrong here.