Bug#489340: iproute2: no error message when link up command fails.

Bug#489340: iproute2: no error message when link up command fails.

[Andreas Henriksson]

Hi Stephen and co.!

Johannes Berg reported that iproute2 doesn't give any error message when
"ip link set ... up" failed for him (as opposed to ifconfig):
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=489340

The ways he suggested didn't work for me to reproduce, but I found out
simply using the wmaster0 device works as a testcase.
(You'll need a wireless device, probably with a driver based on the new
mac80211 stack).

I've debugged this into a place in the bundled rtnetlink library where
if there's a netlink error - it is ignored if there's no errno, which
seems weird. I don't really understand the code, but this "proof of
concept" patch makes "ip link set dev wmaster0 up" spit out an error
message atleast. Could you please have a look at what's going on here?


diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 5ae64f7..afa58fb 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -351,6 +351,7 @@ int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n, pid_t peer,
 					if (errno == 0) {
 						if (answer)
 							memcpy(answer, h, h->nlmsg_len);
+						fprintf(stderr, "Unknown netlink error.\n");
 						return 0;
 					}
 					perror("RTNETLINK answers");



For the record, here's what ifconfig says:

$ sudo ifconfig wmaster0 up
SIOCSIFFLAGS: Operation not supported


--
Regards,
Andreas Henriksson

Re: iproute2: no error message when link up command fails.

[Stephen Hemminger]

On Thu, 17 Jul 2008 00:00:58 +0200
Andreas Henriksson <andreas@fatal.se> wrote:

> Hi Stephen and co.!
> 
> Johannes Berg reported that iproute2 doesn't give any error message when
> "ip link set ... up" failed for him (as opposed to ifconfig):
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=489340
> 
> The ways he suggested didn't work for me to reproduce, but I found out
> simply using the wmaster0 device works as a testcase.
> (You'll need a wireless device, probably with a driver based on the new
> mac80211 stack).
> 
> I've debugged this into a place in the bundled rtnetlink library where
> if there's a netlink error - it is ignored if there's no errno, which
> seems weird. I don't really understand the code, but this "proof of
> concept" patch makes "ip link set dev wmaster0 up" spit out an error
> message atleast. Could you please have a look at what's going on here?
> 
> 
> diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> index 5ae64f7..afa58fb 100644
> --- a/lib/libnetlink.c
> +++ b/lib/libnetlink.c
> @@ -351,6 +351,7 @@ int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n, pid_t peer,
>  					if (errno == 0) {
>  						if (answer)
>  							memcpy(answer, h, h->nlmsg_len);
> +						fprintf(stderr, "Unknown netlink error.\n");
>  						return 0;
>  					}
>  					perror("RTNETLINK answers");
> 
> 
> 
> For the record, here's what ifconfig says:
> 
> $ sudo ifconfig wmaster0 up
> SIOCSIFFLAGS: Operation not supported

libnetlink shouldn't print the error, it needs to be done by the caller.

Re: iproute2: no error message when link up command fails.

[Stephen Hemminger]

On Thu, 17 Jul 2008 00:27:17 +0200
Andreas Henriksson <andreas@fatal.se> wrote:

> On ons, 2008-07-16 at 15:03 -0700, Stephen Hemminger wrote:
> > On Thu, 17 Jul 2008 00:00:58 +0200
> > Andreas Henriksson <andreas@fatal.se> wrote:
> [...]
> > > +						fprintf(stderr, "Unknown netlink error.\n");
> > >  						return 0;
> [..]
> > libnetlink shouldn't print the error, it needs to be done by the caller.
> ... and iproute should exit with a proper error code. This isn't
> possible today, as there's no way for the caller to detect the error!
> I was just trying to be a bit helpful on where we end up in the code.
> 
> If anyone could help out with how to modify the code to solve all this,
> that would be nice. I don't understand the current code tries to do.
> 
> (By the way, most uses of rtnl_* seems to be if (rtnl_* < 0) exit(1); in
> iproute2 currently. The error messages are in libnetlink.)
> 
> 

The problem is the driver is responding with an error packet but the
errno is 0. This looks like a kernel bug, not an library bug.

Re: iproute2: no error message when link up command fails.

[Andreas Henriksson]

On ons, 2008-07-16 at 15:03 -0700, Stephen Hemminger wrote:
> On Thu, 17 Jul 2008 00:00:58 +0200
> Andreas Henriksson <andreas@fatal.se> wrote:
[...]
> > +						fprintf(stderr, "Unknown netlink error.\n");
> >  						return 0;
[..]
> libnetlink shouldn't print the error, it needs to be done by the caller.
... and iproute should exit with a proper error code. This isn't
possible today, as there's no way for the caller to detect the error!
I was just trying to be a bit helpful on where we end up in the code.

If anyone could help out with how to modify the code to solve all this,
that would be nice. I don't understand the current code tries to do.

(By the way, most uses of rtnl_* seems to be if (rtnl_* < 0) exit(1); in
iproute2 currently. The error messages are in libnetlink.)


-- 
Regards,
Andreas Henriksson

Re: iproute2: no error message when link up command fails.

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 443 bytes --]


> > (By the way, most uses of rtnl_* seems to be if (rtnl_* < 0) exit(1); in
> > iproute2 currently. The error messages are in libnetlink.)

> The problem is the driver is responding with an error packet but the
> errno is 0. This looks like a kernel bug, not an library bug.

I don't think so, the recvmsg() call worked fine, but the message
indicates that the netlink consumer had an error. Or am I missing
something?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: iproute2: no error message when link up command fails.

[Stephen Hemminger]

On Thu, 17 Jul 2008 00:35:22 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:

> 
> > > (By the way, most uses of rtnl_* seems to be if (rtnl_* < 0) exit(1); in
> > > iproute2 currently. The error messages are in libnetlink.)
> 
> > The problem is the driver is responding with an error packet but the
> > errno is 0. This looks like a kernel bug, not an library bug.
> 
> I don't think so, the recvmsg() call worked fine, but the message
> indicates that the netlink consumer had an error. Or am I missing
> something?
> 
> johannes

The netlink message in question is marked as type ERROR but the errno
encoded in the message is zero.

		if (h->nlmsg_type == NLMSG_ERROR) {
			struct nlmsgerr *err = (struct nlmsgerr*)NLMSG_DATA(h);
			if (l < sizeof(struct nlmsgerr)) {
				fprintf(stderr, "ERROR truncated\n");
			} else {
				errno = -err->error;
				if (errno == 0) {
					if (answer)
						memcpy(answer, h, h->nlmsg_len);
					return 0;
				}
				perror("RTNETLINK answers");
			}

So the netlink library just treats as a successful return.
To me it looks like the problem is in the kernel sending back
a NLMSG_ERROR with errno of zero. Some code path isn't setting
it up properly.

Re: iproute2: no error message when link up command fails.

[Andreas Henriksson]

On ons, 2008-07-16 at 15:53 -0700, Stephen Hemminger wrote:
> The netlink message in question is marked as type ERROR but the errno
> encoded in the message is zero.
> 
> 		if (h->nlmsg_type == NLMSG_ERROR) {
> 			struct nlmsgerr *err = (struct nlmsgerr*)NLMSG_DATA(h);
> 			if (l < sizeof(struct nlmsgerr)) {
> 				fprintf(stderr, "ERROR truncated\n");
> 			} else {
> 				errno = -err->error;
> 				if (errno == 0) {
> 					if (answer)
> 						memcpy(answer, h, h->nlmsg_len);
> 					return 0;
> 				}
> 				perror("RTNETLINK answers");
> 			}
> 
> So the netlink library just treats as a successful return.
Why? This seems like a really bad idea to me, and none of the callers in
iproute benefits from this as far as I can see.

Just ripping out the errno == 0 special casing looks like and option to
me, unless anyone can find a reason for it.
(It'll give an error message and an error exit code! The message will be
strange, but lets blame the kernel for that cosmetic issue. Atleast the
user got some kind of notification.)

Moving the "return 0;" inside the "if (answer)" would be another
(atleast for iproutes callers of the library functions)...

> To me it looks like the problem is in the kernel sending back
> a NLMSG_ERROR with errno of zero. Some code path isn't setting
> it up properly.

None the less, it would be be good if the application wouldn't poop it's
pants when it can be avoided - broken kernel or not.


diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 5ae64f7..4413165 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -348,12 +348,7 @@ int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n, pid_t peer,
 					fprintf(stderr, "ERROR truncated\n");
 				} else {
 					errno = -err->error;
-					if (errno == 0) {
-						if (answer)
-							memcpy(answer, h, h->nlmsg_len);
-						return 0;
-					}
-					perror("RTNETLINK answers");
+					perror("RTNETLINK error");
 				}
 				return -1;
 			}


-- 
Regards,
Andreas Henriksson

Re: iproute2: no error message when link up command fails.

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]

Andreas Henriksson wrote:
> On ons, 2008-07-16 at 15:53 -0700, Stephen Hemminger wrote:
>> The netlink message in question is marked as type ERROR but the errno
>> encoded in the message is zero.
>>
>> 		if (h->nlmsg_type == NLMSG_ERROR) {
>> 			struct nlmsgerr *err = (struct nlmsgerr*)NLMSG_DATA(h);
>> 			if (l < sizeof(struct nlmsgerr)) {
>> 				fprintf(stderr, "ERROR truncated\n");
>> 			} else {
>> 				errno = -err->error;
>> 				if (errno == 0) {
>> 					if (answer)
>> 						memcpy(answer, h, h->nlmsg_len);
>> 					return 0;
>> 				}
>> 				perror("RTNETLINK answers");
>> 			}
>>
>> So the netlink library just treats as a successful return.
> Why? This seems like a really bad idea to me, and none of the callers in
> iproute benefits from this as far as I can see.
> 
> Just ripping out the errno == 0 special casing looks like and option to
> me, unless anyone can find a reason for it.

NLMSG_ERROR with errno == 0 is a netlink ACK message.

> (It'll give an error message and an error exit code! The message will be
> strange, but lets blame the kernel for that cosmetic issue. Atleast the
> user got some kind of notification.)
> 
> Moving the "return 0;" inside the "if (answer)" would be another
> (atleast for iproutes callers of the library functions)...
> 
>> To me it looks like the problem is in the kernel sending back
>> a NLMSG_ERROR with errno of zero. Some code path isn't setting
>> it up properly.
> 
> None the less, it would be be good if the application wouldn't poop it's
> pants when it can be avoided - broken kernel or not.

The fix in this case is to propagate the return value from
dev_change_flags().



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 784 bytes --]

rtnetlink: propagate error from dev_change_flags in do_setlink()

Andreas Henriksson <andreas@fatal.se> reported that unlike ifconfig,
iproute doesn't report an error when setting an interface up fails.

Propagate the return value from dev_change_flags() to fix this.

Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a9a7721..ffde766 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -867,7 +867,9 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 		if (ifm->ifi_change)
 			flags = (flags & ifm->ifi_change) |
 				(dev->flags & ~ifm->ifi_change);
-		dev_change_flags(dev, flags);
+		err = dev_change_flags(dev, flags);
+		if (err < 0)
+			goto errout;
 	}
 
 	if (tb[IFLA_TXQLEN])

Re: iproute2: no error message when link up command fails.

[Jarek Poplawski]

On 17-07-2008 11:26, Patrick McHardy wrote:
...
> rtnetlink: propagate error from dev_change_flags in do_setlink()
> 
> Andreas Henriksson <andreas@fatal.se> reported that

+ Johannes Berg reported that

> unlike ifconfig,
> iproute doesn't report an error when setting an interface up fails.
> 
> Propagate the return value from dev_change_flags() to fix this.
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Jarek P.

Re: iproute2: no error message when link up command fails.

[jamal]

On Thu, 2008-17-07 at 02:31 +0200, Andreas Henriksson wrote:

> Why? 

Thats just how it rolls.

> This seems like a really bad idea to me, and none of the callers in
> iproute benefits from this as far as I can see.

The receiver(kernel in this case, but it could be some other user space
user) returning a zero means success. Essentially zero is an (Positive)
ACK.
The receiver returning a non-zero implies a failure. Essentially a
N(egative) ACK. 

In the case of a NACK, the kernel must return you the original message
header you sent (similar to the way some icmp messages behave).
The returned error code is a standard errno - if you sent a bad config
you may get an EINVAL back. The sender combines the errno + the header
to figure out what went wrong.

Does that make sense? So the kernel fix is required (as Stephen noted).

cheers,
jamal