On 24/01/2018 16:44, Stephen Hemminger wrote: > On Wed, 24 Jan 2018 10:19:24 +0100 > Phil Sutter <p...@nwl.cc> wrote: > >> Hi Stephen, >> >> On Tue, Jan 23, 2018 at 02:44:42PM -0800, Stephen Hemminger wrote: >>> On Tue, 23 Jan 2018 17:40:47 +0100 >>> Phil Sutter <p...@nwl.cc> wrote: >>> >>>> The following command segfaults if enp0s31f6 does not exist: >>>> >>>> | # ip -6 route add default proto ra metric 20100 \ >>>> | nexthop via fe80:52:0:2040::1fc dev enp0s31f6 weight 1 \ >>>> | nexthop via fe80:52:0:2040::1fe dev enp0s31f6 weight 1 >>>> >>>> Since the non-zero return code from parse_one_nh() is ignored, >>>> parse_nexthops() continues iterating over the the same fields in argv >>>> until buffer space is exhausted and eventually accesses unallocated >>>> memory. >>>> >>>> Fix this by aborting on error in parse_nexthops() and make >>>> iproute_modify() fail if parse_nexthops() did. >>>> >>>> Reported-by: Lennart Poettering <lpoet...@redhat.com> >>>> Fixes: 2f406f2d0b4ef ("ip route: replace exits with returns") >>>> Signed-off-by: Phil Sutter <p...@nwl.cc> >>>> --- >>>> ip/iproute.c | 7 ++++--- >>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/ip/iproute.c b/ip/iproute.c >>>> index bf886fda9d761..d7accf57ac8d1 100644 >>>> --- a/ip/iproute.c >>>> +++ b/ip/iproute.c >>>> @@ -871,7 +871,8 @@ static int parse_nexthops(struct nlmsghdr *n, struct >>>> rtmsg *r, >>>> memset(rtnh, 0, sizeof(*rtnh)); >>>> rtnh->rtnh_len = sizeof(*rtnh); >>>> rta->rta_len += rtnh->rtnh_len; >>>> - parse_one_nh(n, r, rta, rtnh, &argc, &argv); >>>> + if (parse_one_nh(n, r, rta, rtnh, &argc, &argv) < 0) >>>> + return -1; >>>> rtnh = RTNH_NEXT(rtnh); >>>> } >>>> >>>> @@ -1318,8 +1319,8 @@ static int iproute_modify(int cmd, unsigned int >>>> flags, int argc, char **argv) >>>> addattr_l(&req.n, sizeof(req), RTA_METRICS, RTA_DATA(mxrta), >>>> RTA_PAYLOAD(mxrta)); >>>> } >>>> >>>> - if (nhs_ok) >>>> - parse_nexthops(&req.n, &req.r, argc, argv); >>>> + if (nhs_ok && parse_nexthops(&req.n, &req.r, argc, argv) < 0) >>>> + return -1; >>>> >>>> if (req.r.rtm_family == AF_UNSPEC) >>>> req.r.rtm_family = AF_INET; >>> >>> >>> The real issue is that handling of invalid device is different than all the >>> other >>> possible semantic errors. >>> >>> My recommendations are: >>> * change bad device to use invarg() which does exit >>> * make functions that only return 0 void including >>> parse_one_nh >>> lwt_parse_encap >>> get_addr >>> >>> Also, it looks like read_family converts any address family it doesn't know >>> about to unspec >>> that is stupid behavior as well. >>> >>> The original commit 2f406f2d0b4ef ("ip route: replace exits with returns") >>> looks like well intentioned but suspect. Most of the errors in ip route >>> indicate real issues where continuing is not a good plan. >> >> You're right, the use of invarg() for any other error effectively >> prevents what said commit tried to achieve, so my fix is pretty >> pointless in that regard. Yet I wonder why we still have 'ip -batch >> -force' given that it's not useful. Maybe Élie is able to provide some >> details about the use-case said commit tried to fix? >> >> Meanwhile I'll prepare some patches to address the shortcomings you >> mentioned above. > > The use case for batch (and force) is that there may be a large set of routes > or qdisc operations where it is ok for some of them to fail because of > responses > from the kernel failing. I don't think batch should ever just continue if > handed > invalid syntax for device or address. There are some borderline cases, for > example > if a tunnel device could not be created and later steps depend on that name. > > Agree, lets get some real data on why the original patch was done.
If I remember correctly, I made this commit for a batch of "route get" and not stop on inexistent routes. But my patch was not limited to this use case and I tried to fix others potential situations where we should not stop. The change I made in parse_one_nh is not directly related to my use care, sorry for the introduced regression, I should have been more careful. Ihmo, if a tunnel device could not be created, later steps depending on it will fail too, but we potentially still want subsequent operations (for instance the creation of a second tunnel) to be done. I you don't want it, don't use -force or make two batch files. From this point of view, Phil patch is better than invarg() but I'm fine with invarg() too. Thank you all and sorry again, Élie