Re: [PATCH iproute2] mptcp: avoid uninitialised errno usage

[David Ahern <dsahern@gmail.com>]

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;
>  }
>  
>