On 6/3/18 8:31 AM, Eric Dumazet wrote:
>
>
> On 06/03/2018 07:01 AM, David Ahern wrote:
>> On 6/3/18 7:35 AM, Eric Dumazet wrote:
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index
>>> f4d61736c41abe8cd7f439c4a37100e90c1eacca..830eefdbdb6734eb81ea0322fb6077ee20be1889
>>> 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -4263,7 +4263,9 @@ static int ip6_route_multipath_add(struct fib6_config
>>> *cfg,
>>>
>>> err_nh = NULL;
>>> list_for_each_entry(nh, &rt6_nh_list, next) {
>>> + dst_release(&rt_last->dst);
>>> rt_last = nh->rt6_info;
>>> + dst_hold(&rt_last->dst);
>>> err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack);
>>> /* save reference to first route for notification */
>>> if (!rt_notif && !err)
>>> @@ -4317,7 +4319,7 @@ static int ip6_route_multipath_add(struct fib6_config
>>> *cfg,
>>> list_del(&nh->next);
>>> kfree(nh);
>>> }
>>> -
>>> + dst_release(&rt_last->dst);
>>> return err;
>>> }
>>
>> Since the rtnl lock is held, a successfully inserted route can not be
>> removed until ip6_route_multipath_add finishes. This is a simpler change
>> that works with net-next as well:
>
> Your patch changes the intent of your original commit.
>
> It seems you wanted rt_last to point to the last attempted insertion,
> not the last successful one ?
The note in ip6_route_mpath_notify explains it:
/* if this is an APPEND route, then rt points to the first route
* inserted and rt_last points to last route inserted. Userspace
>
> Or have I misunderstood, and not only we had a use-after-free, but also
> a semantic error ?
It was a mistake to set rt_last before checking err. So the
use-after-free exposed the semantic error.
