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.
