* [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).