* [PATCH] Do not listen if rtnl_send() fails in ip link iplink_have_newlink() test
@ 2013-12-11 13:01 Petr Písař
2013-12-11 18:57 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Petr Písař @ 2013-12-11 13:01 UTC (permalink / raw)
To: netdev; +Cc: Stephen Hemminger, Petr Písař
If rtnl_send() fails in iplink_have_newlink() test, listening for
response will result in indefinite hang. This can be demonstrated by
"ip link show" while SELinux preventing from sending the RTM_NEWLINK
over netlink.
This patch checks for the return value as is done at all other
rtnl_send() calls.
Signed-off-by: Petr Písař <ppisar@redhat.com>
---
ip/iplink.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ip/iplink.c b/ip/iplink.c
index 58b6c20..f7d9e17 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -178,8 +178,8 @@ static int iplink_have_newlink(void)
req.n.nlmsg_type = RTM_NEWLINK;
req.i.ifi_family = AF_UNSPEC;
- rtnl_send(&rth, &req.n, req.n.nlmsg_len);
- rtnl_listen(&rth, accept_msg, NULL);
+ if (rtnl_send(&rth, &req.n, req.n.nlmsg_len) >= 0)
+ rtnl_listen(&rth, accept_msg, NULL);
}
return have_rtnl_newlink;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Do not listen if rtnl_send() fails in ip link iplink_have_newlink() test
2013-12-11 13:01 [PATCH] Do not listen if rtnl_send() fails in ip link iplink_have_newlink() test Petr Písař
@ 2013-12-11 18:57 ` Stephen Hemminger
2013-12-12 7:18 ` Petr Písař
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2013-12-11 18:57 UTC (permalink / raw)
To: Petr Písař; +Cc: netdev
On Wed, 11 Dec 2013 14:01:29 +0100
Petr Písař <ppisar@redhat.com> wrote:
> If rtnl_send() fails in iplink_have_newlink() test, listening for
> response will result in indefinite hang. This can be demonstrated by
> "ip link show" while SELinux preventing from sending the RTM_NEWLINK
> over netlink.
>
> This patch checks for the return value as is done at all other
> rtnl_send() calls.
>
> Signed-off-by: Petr Písař <ppisar@redhat.com>
> ---
> ip/iplink.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ip/iplink.c b/ip/iplink.c
> index 58b6c20..f7d9e17 100644
> --- a/ip/iplink.c
> +++ b/ip/iplink.c
> @@ -178,8 +178,8 @@ static int iplink_have_newlink(void)
> req.n.nlmsg_type = RTM_NEWLINK;
> req.i.ifi_family = AF_UNSPEC;
>
> - rtnl_send(&rth, &req.n, req.n.nlmsg_len);
> - rtnl_listen(&rth, accept_msg, NULL);
> + if (rtnl_send(&rth, &req.n, req.n.nlmsg_len) >= 0)
> + rtnl_listen(&rth, accept_msg, NULL);
> }
> return have_rtnl_newlink;
> }
I think it should print an error messag, not silently ignore the send failure.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Do not listen if rtnl_send() fails in ip link iplink_have_newlink() test
2013-12-11 18:57 ` Stephen Hemminger
@ 2013-12-12 7:18 ` Petr Písař
2013-12-20 16:21 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Petr Písař @ 2013-12-12 7:18 UTC (permalink / raw)
To: netdev; +Cc: Stephen Hemminger, Petr Písař
If rtnl_send() fails in iplink_have_newlink() test, listening for
response will result in indefinite hang. This can be demonstrated by
"ip link show" while SELinux preventing from sending the RTM_NEWLINK
over netlink.
This patch checks for the return value as is done at all other
rtnl_send() calls. It falls back to IOCTL in case of failure.
Signed-off-by: Petr Písař <ppisar@redhat.com>
---
ip/iplink.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/ip/iplink.c b/ip/iplink.c
index 58b6c20..5132868 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -178,8 +178,13 @@ static int iplink_have_newlink(void)
req.n.nlmsg_type = RTM_NEWLINK;
req.i.ifi_family = AF_UNSPEC;
- rtnl_send(&rth, &req.n, req.n.nlmsg_len);
- rtnl_listen(&rth, accept_msg, NULL);
+ if (rtnl_send(&rth, &req.n, req.n.nlmsg_len) < 0) {
+ perror("Could not check for "
+ "link configuration over netlink support");
+ have_rtnl_newlink = 0;
+ } else {
+ rtnl_listen(&rth, accept_msg, NULL);
+ }
}
return have_rtnl_newlink;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Do not listen if rtnl_send() fails in ip link iplink_have_newlink() test
2013-12-12 7:18 ` Petr Písař
@ 2013-12-20 16:21 ` Stephen Hemminger
2014-01-02 7:42 ` Petr Pisar
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2013-12-20 16:21 UTC (permalink / raw)
To: Petr Písař; +Cc: netdev
I took your idea and enhanced it to all of iproute2 by doing the following:
From c4b6330a3a033bd9c9b0664c5f844493137ae599 Mon Sep 17 00:00:00 2001
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Fri, 20 Dec 2013 08:15:02 -0800
Subject: [PATCH] check return value of rtnl_send and related functions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Use warn_unused_result to enforce checking return value of rtnl_send,
and fix where the errors are.
Suggested by initial patch from Petr Písař <ppisar@redhat.com>
---
include/libnetlink.h | 28 ++++++++++++++++++++--------
ip/iplink.c | 5 ++++-
ip/ipnetconf.c | 5 ++++-
misc/arpd.c | 6 +++++-
misc/ss.c | 4 +++-
5 files changed, 36 insertions(+), 12 deletions(-)
diff --git a/include/libnetlink.h b/include/libnetlink.h
index ec3d657..fe7d5d3 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -22,13 +22,22 @@ struct rtnl_handle
extern int rcvbuf;
-extern int rtnl_open(struct rtnl_handle *rth, unsigned subscriptions);
-extern int rtnl_open_byproto(struct rtnl_handle *rth, unsigned subscriptions, int protocol);
+extern int rtnl_open(struct rtnl_handle *rth, unsigned subscriptions)
+ __attribute__((warn_unused_result));
+
+extern int rtnl_open_byproto(struct rtnl_handle *rth, unsigned subscriptions,
+ int protocol)
+ __attribute__((warn_unused_result));
+
extern void rtnl_close(struct rtnl_handle *rth);
-extern int rtnl_wilddump_request(struct rtnl_handle *rth, int fam, int type);
+extern int rtnl_wilddump_request(struct rtnl_handle *rth, int fam, int type)
+ __attribute__((warn_unused_result));
extern int rtnl_wilddump_req_filter(struct rtnl_handle *rth, int fam, int type,
- __u32 filt_mask);
-extern int rtnl_dump_request(struct rtnl_handle *rth, int type, void *req, int len);
+ __u32 filt_mask)
+ __attribute__((warn_unused_result));
+extern int rtnl_dump_request(struct rtnl_handle *rth, int type, void *req,
+ int len)
+ __attribute__((warn_unused_result));
typedef int (*rtnl_filter_t)(const struct sockaddr_nl *,
struct nlmsghdr *n, void *);
@@ -44,9 +53,12 @@ extern int rtnl_dump_filter_l(struct rtnl_handle *rth,
extern int rtnl_dump_filter(struct rtnl_handle *rth, rtnl_filter_t filter,
void *arg);
extern int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n, pid_t peer,
- unsigned groups, struct nlmsghdr *answer);
-extern int rtnl_send(struct rtnl_handle *rth, const void *buf, int);
-extern int rtnl_send_check(struct rtnl_handle *rth, const void *buf, int);
+ unsigned groups, struct nlmsghdr *answer)
+ __attribute__((warn_unused_result));
+extern int rtnl_send(struct rtnl_handle *rth, const void *buf, int)
+ __attribute__((warn_unused_result));
+extern int rtnl_send_check(struct rtnl_handle *rth, const void *buf, int)
+ __attribute__((warn_unused_result));
extern int addattr(struct nlmsghdr *n, int maxlen, int type);
extern int addattr8(struct nlmsghdr *n, int maxlen, int type, __u8 data);
diff --git a/ip/iplink.c b/ip/iplink.c
index 58b6c20..e0c14e6 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -178,7 +178,10 @@ static int iplink_have_newlink(void)
req.n.nlmsg_type = RTM_NEWLINK;
req.i.ifi_family = AF_UNSPEC;
- rtnl_send(&rth, &req.n, req.n.nlmsg_len);
+ if (rtnl_send(&rth, &req.n, req.n.nlmsg_len) < 0) {
+ perror("request send failed");
+ exit(1);
+ }
rtnl_listen(&rth, accept_msg, NULL);
}
return have_rtnl_newlink;
diff --git a/ip/ipnetconf.c b/ip/ipnetconf.c
index 9a77ecb..37aaf45 100644
--- a/ip/ipnetconf.c
+++ b/ip/ipnetconf.c
@@ -161,7 +161,10 @@ static int do_show(int argc, char **argv)
addattr_l(&req.n, sizeof(req), NETCONFA_IFINDEX,
&filter.ifindex, sizeof(filter.ifindex));
- rtnl_send(&rth, &req.n, req.n.nlmsg_len);
+ if (rtnl_send(&rth, &req.n, req.n.nlmsg_len) < 0) {
+ perror("Can not send request");
+ exit(1);
+ }
rtnl_listen(&rth, print_netconf, stdout);
} else {
dump:
diff --git a/misc/arpd.c b/misc/arpd.c
index ec9d570..d293b70 100644
--- a/misc/arpd.c
+++ b/misc/arpd.c
@@ -428,7 +428,11 @@ static int do_one_request(struct nlmsghdr *n)
static void load_initial_table(void)
{
- rtnl_wilddump_request(&rth, AF_INET, RTM_GETNEIGH);
+ if (rtnl_wilddump_request(&rth, AF_INET, RTM_GETNEIGH) < 0) {
+ perrror("dump request failed");
+ exit(1);
+ }
+
}
static void get_kern_msg(void)
diff --git a/misc/ss.c b/misc/ss.c
index 6f38ae7..e59ca5c 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -996,7 +996,9 @@ static int xll_initted = 0;
static void xll_init(void)
{
struct rtnl_handle rth;
- rtnl_open(&rth, 0);
+ if (rtnl_open(&rth, 0) < 0)
+ exit(1);
+
ll_init_map(&rth);
rtnl_close(&rth);
xll_initted = 1;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Do not listen if rtnl_send() fails in ip link iplink_have_newlink() test
2013-12-20 16:21 ` Stephen Hemminger
@ 2014-01-02 7:42 ` Petr Pisar
2014-01-03 20:36 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Petr Pisar @ 2014-01-02 7:42 UTC (permalink / raw)
To: netdev; +Cc: Stephen Hemminger
[-- Attachment #1: Type: text/plain, Size: 1356 bytes --]
On Fri, Dec 20, 2013 at 08:21:07AM -0800, Stephen Hemminger wrote:
> I took your idea and enhanced it to all of iproute2 by doing the following:
>
> From c4b6330a3a033bd9c9b0664c5f844493137ae599 Mon Sep 17 00:00:00 2001
> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Fri, 20 Dec 2013 08:15:02 -0800
> Subject: [PATCH] check return value of rtnl_send and related functions
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Use warn_unused_result to enforce checking return value of rtnl_send,
> and fix where the errors are.
>
[...]
> diff --git a/ip/iplink.c b/ip/iplink.c
> index 58b6c20..e0c14e6 100644
> --- a/ip/iplink.c
> +++ b/ip/iplink.c
> @@ -178,7 +178,10 @@ static int iplink_have_newlink(void)
> req.n.nlmsg_type = RTM_NEWLINK;
> req.i.ifi_family = AF_UNSPEC;
>
> - rtnl_send(&rth, &req.n, req.n.nlmsg_len);
> + if (rtnl_send(&rth, &req.n, req.n.nlmsg_len) < 0) {
> + perror("request send failed");
> + exit(1);
> + }
> rtnl_listen(&rth, accept_msg, NULL);
> }
> return have_rtnl_newlink;
This one exits instead of falling back to IOCTL. iplink_have_newlink() is
called even from iplink_usage(). This even prevents from printing usage.
I think failure in this very place should be treated as I suggested.
-- Petr
[-- Attachment #2: Type: application/pgp-signature, Size: 230 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Do not listen if rtnl_send() fails in ip link iplink_have_newlink() test
2014-01-02 7:42 ` Petr Pisar
@ 2014-01-03 20:36 ` Stephen Hemminger
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Hemminger @ 2014-01-03 20:36 UTC (permalink / raw)
To: Petr Pisar; +Cc: netdev
On Thu, 2 Jan 2014 08:42:57 +0100
Petr Pisar <ppisar@redhat.com> wrote:
> On Fri, Dec 20, 2013 at 08:21:07AM -0800, Stephen Hemminger wrote:
> > I took your idea and enhanced it to all of iproute2 by doing the following:
> >
> > From c4b6330a3a033bd9c9b0664c5f844493137ae599 Mon Sep 17 00:00:00 2001
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Date: Fri, 20 Dec 2013 08:15:02 -0800
> > Subject: [PATCH] check return value of rtnl_send and related functions
> > MIME-Version: 1.0
> > Content-Type: text/plain; charset=UTF-8
> > Content-Transfer-Encoding: 8bit
> >
> > Use warn_unused_result to enforce checking return value of rtnl_send,
> > and fix where the errors are.
> >
> [...]
> > diff --git a/ip/iplink.c b/ip/iplink.c
> > index 58b6c20..e0c14e6 100644
> > --- a/ip/iplink.c
> > +++ b/ip/iplink.c
> > @@ -178,7 +178,10 @@ static int iplink_have_newlink(void)
> > req.n.nlmsg_type = RTM_NEWLINK;
> > req.i.ifi_family = AF_UNSPEC;
> >
> > - rtnl_send(&rth, &req.n, req.n.nlmsg_len);
> > + if (rtnl_send(&rth, &req.n, req.n.nlmsg_len) < 0) {
> > + perror("request send failed");
> > + exit(1);
> > + }
> > rtnl_listen(&rth, accept_msg, NULL);
> > }
> > return have_rtnl_newlink;
>
> This one exits instead of falling back to IOCTL. iplink_have_newlink() is
> called even from iplink_usage(). This even prevents from printing usage.
>
> I think failure in this very place should be treated as I suggested.
>
> -- Petr
The code gets used in multiple paths, some should fail and others have
fallback. I will fix the cases where fallback is possible.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-01-03 20:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-11 13:01 [PATCH] Do not listen if rtnl_send() fails in ip link iplink_have_newlink() test Petr Písař
2013-12-11 18:57 ` Stephen Hemminger
2013-12-12 7:18 ` Petr Písař
2013-12-20 16:21 ` Stephen Hemminger
2014-01-02 7:42 ` Petr Pisar
2014-01-03 20:36 ` Stephen Hemminger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).