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 ?
Or have I misunderstood, and not only we had a use-after-free, but also
a semantic error ?
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index f4d61736c41a..1684197c189f 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -4263,11 +4263,12 @@ static int ip6_route_multipath_add(struct
> fib6_config *cfg,
>
> err_nh = NULL;
> list_for_each_entry(nh, &rt6_nh_list, next) {
> - rt_last = nh->rt6_info;
> err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack);
> /* save reference to first route for notification */
> if (!rt_notif && !err)
> rt_notif = nh->rt6_info;
> + if (!err)
> + rt_last = nh->rt6_info;
>
> /* nh->rt6_info is used or freed at this point, reset to
> NULL*/
> nh->rt6_info = NULL;
>
>
> Is there a reproducer for the syzbot case?
Not yet.
