| Message ID | 20210503103631.30694-1-fw@strlen.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2] mptcp: avoid uninitialised errno usage | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 5/3/21 4:36 AM, Florian Westphal wrote: > The function called *might* set errno based on errno value in NLMSG_ERROR > message, but in case no such message exists errno is left alone. > > This may cause ip to fail with > "can't subscribe to mptcp events: Success" > > on kernels that support mptcp but lack event support (all kernels <= 5.11). > > Set errno to a meaningful value. This will then still exit with the > more specific 'permission denied' or some such when called by process > that lacks CAP_NET_ADMIN on 5.12 and later. > > Fixes: ff619e4fd370 ("mptcp: add support for event monitoring") > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > ip/ipmptcp.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c > index 5f490f0026d9..504b5b2f5329 100644 > --- a/ip/ipmptcp.c > +++ b/ip/ipmptcp.c > @@ -491,6 +491,9 @@ out: > > static int mptcp_monitor(void) > { > + /* genl_add_mcast_grp may or may not set errno */ > + errno = EOPNOTSUPP; > + > if (genl_add_mcast_grp(&genl_rth, genl_family, MPTCP_PM_EV_GRP_NAME) < 0) { > perror("can't subscribe to mptcp events"); > return 1; > Seems like this should be set in genl_add_mcast_grp vs its caller.
David Ahern <dsahern@gmail.com> wrote: > On 5/3/21 4:36 AM, Florian Westphal wrote: > > The function called *might* set errno based on errno value in NLMSG_ERROR > > message, but in case no such message exists errno is left alone. > > > > This may cause ip to fail with > > "can't subscribe to mptcp events: Success" > > > > on kernels that support mptcp but lack event support (all kernels <= 5.11). > > > > Set errno to a meaningful value. This will then still exit with the > > more specific 'permission denied' or some such when called by process > > that lacks CAP_NET_ADMIN on 5.12 and later. > > > > Fixes: ff619e4fd370 ("mptcp: add support for event monitoring") > > Signed-off-by: Florian Westphal <fw@strlen.de> > > --- > > ip/ipmptcp.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c > > index 5f490f0026d9..504b5b2f5329 100644 > > --- a/ip/ipmptcp.c > > +++ b/ip/ipmptcp.c > > @@ -491,6 +491,9 @@ out: > > > > static int mptcp_monitor(void) > > { > > + /* genl_add_mcast_grp may or may not set errno */ > > + errno = EOPNOTSUPP; > > + > > if (genl_add_mcast_grp(&genl_rth, genl_family, MPTCP_PM_EV_GRP_NAME) < 0) { > > perror("can't subscribe to mptcp events"); > > return 1; > > > > Seems like this should be set in genl_add_mcast_grp vs its caller. I think setting errno in libraries (libc excluded) is a bad design choice. If you still disagree, then I can respin, but it would get a bit more ugly, e.g. (untested!): diff --git a/lib/libgenl.c b/lib/libgenl.c --- a/lib/libgenl.c +++ b/lib/libgenl.c @@ -100,20 +100,29 @@ int genl_add_mcast_grp(struct rtnl_handle *grth, __u16 fnum, const char *group) addattr16(&req.n, sizeof(req), CTRL_ATTR_FAMILY_ID, fnum); + /* clear errno to set a default value if needed */ + errno = 0; + if (rtnl_talk(grth, &req.n, &answer) < 0) { fprintf(stderr, "Error talking to the kernel\n"); + if (errno == 0) + errno = EOPNOTSUPP; return -2; } ghdr = NLMSG_DATA(answer); len = answer->nlmsg_len; - if (answer->nlmsg_type != GENL_ID_CTRL) + if (answer->nlmsg_type != GENL_ID_CTRL) { + errno = EINVAL; goto err_free; + } len -= NLMSG_LENGTH(GENL_HDRLEN); - if (len < 0) + if (len < 0) { + errno = EINVAL; goto err_free; + } attrs = (struct rtattr *) ((char *) ghdr + GENL_HDRLEN); parse_rtattr(tb, CTRL_ATTR_MAX, attrs, len); @@ -130,6 +139,10 @@ int genl_add_mcast_grp(struct rtnl_handle *grth, __u16 fnum, const char *group) err_free: free(answer); + + if (ret < 0 && errno == 0) + errno = EOPNOTSUPP; + return ret; }
On 5/9/21 4:25 PM, Florian Westphal wrote: > David Ahern <dsahern@gmail.com> wrote: >> On 5/3/21 4:36 AM, Florian Westphal wrote: >>> The function called *might* set errno based on errno value in NLMSG_ERROR >>> message, but in case no such message exists errno is left alone. >>> >>> This may cause ip to fail with >>> "can't subscribe to mptcp events: Success" >>> >>> on kernels that support mptcp but lack event support (all kernels <= 5.11). >>> >>> Set errno to a meaningful value. This will then still exit with the >>> more specific 'permission denied' or some such when called by process >>> that lacks CAP_NET_ADMIN on 5.12 and later. >>> >>> Fixes: ff619e4fd370 ("mptcp: add support for event monitoring") >>> Signed-off-by: Florian Westphal <fw@strlen.de> >>> --- >>> ip/ipmptcp.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c >>> index 5f490f0026d9..504b5b2f5329 100644 >>> --- a/ip/ipmptcp.c >>> +++ b/ip/ipmptcp.c >>> @@ -491,6 +491,9 @@ out: >>> >>> static int mptcp_monitor(void) >>> { >>> + /* genl_add_mcast_grp may or may not set errno */ >>> + errno = EOPNOTSUPP; >>> + >>> if (genl_add_mcast_grp(&genl_rth, genl_family, MPTCP_PM_EV_GRP_NAME) < 0) { >>> perror("can't subscribe to mptcp events"); >>> return 1; >>> >> >> Seems like this should be set in genl_add_mcast_grp vs its caller. > > I think setting errno in libraries (libc excluded) is a bad design lib/libnetlink.c, rtnl_talk for example already does. I think it would be best for the location of the error to set the errno. Your suggested change here goes way beyond setting a default errno before calling genl_add_mcast_grp. > choice. If you still disagree, then I can respin, but it would get a > bit more ugly, e.g. (untested!): > > diff --git a/lib/libgenl.c b/lib/libgenl.c > --- a/lib/libgenl.c > +++ b/lib/libgenl.c > @@ -100,20 +100,29 @@ int genl_add_mcast_grp(struct rtnl_handle *grth, __u16 fnum, const char *group) > > addattr16(&req.n, sizeof(req), CTRL_ATTR_FAMILY_ID, fnum); > > + /* clear errno to set a default value if needed */ > + errno = 0; > + > if (rtnl_talk(grth, &req.n, &answer) < 0) { > fprintf(stderr, "Error talking to the kernel\n"); > + if (errno == 0) > + errno = EOPNOTSUPP; you don't list the above string in the output in the commit log. Staring at rtnl_talk and recvmsg and its failure paths, it seems unlikely that path is causing the problem. > return -2; > } > > ghdr = NLMSG_DATA(answer); > len = answer->nlmsg_len; > > - if (answer->nlmsg_type != GENL_ID_CTRL) > + if (answer->nlmsg_type != GENL_ID_CTRL) { > + errno = EINVAL; > goto err_free; > + } > > len -= NLMSG_LENGTH(GENL_HDRLEN); > - if (len < 0) > + if (len < 0) { > + errno = EINVAL; > goto err_free; > + } EINVAL here is different than what you have in the commit message. Are one of these the location of the real problem? Seems likely since your commit log only showed "can't subscribe to mptcp events: Success" and not any other error strings. e.g., if CTRL_ATTR_MCAST_GROUPS is NULL, that would be the place to put the EOPNOTSUPP, but then it too has a string not listed in your commit log. > > attrs = (struct rtattr *) ((char *) ghdr + GENL_HDRLEN); > parse_rtattr(tb, CTRL_ATTR_MAX, attrs, len); > @@ -130,6 +139,10 @@ int genl_add_mcast_grp(struct rtnl_handle *grth, __u16 fnum, const char *group) > > err_free: > free(answer); > + > + if (ret < 0 && errno == 0) > + errno = EOPNOTSUPP; > + > return ret; > } > >
David Ahern <dsahern@gmail.com> wrote: > > if (rtnl_talk(grth, &req.n, &answer) < 0) { > > fprintf(stderr, "Error talking to the kernel\n"); > > + if (errno == 0) > > + errno = EOPNOTSUPP; > > you don't list the above string in the output in the commit log. Staring > at rtnl_talk and recvmsg and its failure paths, it seems unlikely that > path is causing the problem. Its not in my particular case, but if it would caller would still get random errno. The sketch I sent merely provides a relible errno whenever ret is less than 0. Right now it may or may not have been set.
On 5/9/21 4:55 PM, Florian Westphal wrote: > David Ahern <dsahern@gmail.com> wrote: >>> if (rtnl_talk(grth, &req.n, &answer) < 0) { >>> fprintf(stderr, "Error talking to the kernel\n"); >>> + if (errno == 0) >>> + errno = EOPNOTSUPP; >> >> you don't list the above string in the output in the commit log. Staring >> at rtnl_talk and recvmsg and its failure paths, it seems unlikely that >> path is causing the problem. > > Its not in my particular case, but if it would caller would still get random errno. > > The sketch I sent merely provides a relible errno whenever ret is less > than 0. Right now it may or may not have been set. > Then let's find and fix those locations. Unless I missed a code path, rtnl_talk and friends (e.g., __rtnl_talk_iov) all set errno before any < 0 return.
diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c index 5f490f0026d9..504b5b2f5329 100644 --- a/ip/ipmptcp.c +++ b/ip/ipmptcp.c @@ -491,6 +491,9 @@ out: static int mptcp_monitor(void) { + /* genl_add_mcast_grp may or may not set errno */ + errno = EOPNOTSUPP; + if (genl_add_mcast_grp(&genl_rth, genl_family, MPTCP_PM_EV_GRP_NAME) < 0) { perror("can't subscribe to mptcp events"); return 1;
The function called *might* set errno based on errno value in NLMSG_ERROR message, but in case no such message exists errno is left alone. This may cause ip to fail with "can't subscribe to mptcp events: Success" on kernels that support mptcp but lack event support (all kernels <= 5.11). Set errno to a meaningful value. This will then still exit with the more specific 'permission denied' or some such when called by process that lacks CAP_NET_ADMIN on 5.12 and later. Fixes: ff619e4fd370 ("mptcp: add support for event monitoring") Signed-off-by: Florian Westphal <fw@strlen.de> --- ip/ipmptcp.c | 3 +++ 1 file changed, 3 insertions(+)