[PATCH net] route: add rcu_read_lock when lookup route and update fnhe in __ip_do_redirect

[PATCH net] route: add rcu_read_lock when lookup route and update fnhe in __ip_do_redirect

[Xin Long]

The only free exceptions is free_nh_exceptions():
  free_fib_info_rcu()->free_nh_exceptions()
It is in call_rcu():
 free_fib_info():
      call_rcu(&fi->rcu, free_fib_info_rcu);
There is no issue, because it's protected by fib_info rcu.

In ip_route_input(output)_slow:
 rcu_read_lock()
 fib_lookup() [check fib_info dead]
 __mkroute_input(ouput) -> find_exception()
 rcu_read_unlock()
Also safe.

The same thing is done in __ip_rt_update_pmtu():
     rcu_read_lock();
     if (fib_lookup(dev_net(dst->dev), fl4, &res, 0) == 0) {
         struct fib_nh *nh = &FIB_RES_NH(res);

         update_or_create_fnhe(nh, fl4->daddr, 0, mtu,
                       jiffies + ip_rt_mtu_expires);
     }
     rcu_read_unlock();

But there may be an issue in __ip_do_redirect():
         } else {
             if (fib_lookup(net, fl4, &res, 0) == 0) {
                 struct fib_nh *nh = &FIB_RES_NH(res);

                 update_or_create_fnhe(nh, fl4->daddr, new_gw,
                               0, 0);
             }

Which is not running in rcu_read_lock(), it may update a fnhe that
has been freed.

So fix it by adding rcu_read_lock() just like other parts.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/ipv4/route.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 85f184e..08b9e6c 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -751,12 +751,14 @@ static void __ip_do_redirect(struct rtable *rt, struct sk_buff *skb, struct flow
 		if (!(n->nud_state & NUD_VALID)) {
 			neigh_event_send(n, NULL);
 		} else {
+			rcu_read_lock();
 			if (fib_lookup(net, fl4, &res, 0) == 0) {
 				struct fib_nh *nh = &FIB_RES_NH(res);
 
 				update_or_create_fnhe(nh, fl4->daddr, new_gw,
 						      0, 0);
 			}
+			rcu_read_unlock();
 			if (kill_route)
 				rt->dst.obsolete = DST_OBSOLETE_KILL;
 			call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
-- 
2.1.0

Re: [PATCH net] route: add rcu_read_lock when lookup route and update fnhe in __ip_do_redirect

[Eric Dumazet]

On Mon, 2016-02-01 at 18:22 +0800, Xin Long wrote:

> But there may be an issue in __ip_do_redirect():
>          } else {
>              if (fib_lookup(net, fl4, &res, 0) == 0) {
>                  struct fib_nh *nh = &FIB_RES_NH(res);
> 
>                  update_or_create_fnhe(nh, fl4->daddr, new_gw,
>                                0, 0);
>              }
> 
> Which is not running in rcu_read_lock(), it may update a fnhe that
> has been freed.
> 
> So fix it by adding rcu_read_lock() just like other parts.

But the whole __ip_do_redirect() needs to be called from rcu_read_lock()
already.

No need to add another rcu_read_lock() around fib_lookup()

How did you come doing this patch ? Any particular stack dump or
something ?

Re: [PATCH net] route: add rcu_read_lock when lookup route and update fnhe in __ip_do_redirect

[Xin Long]

>
> But the whole __ip_do_redirect() needs to be called from rcu_read_lock()
> already.
>
> No need to add another rcu_read_lock() around fib_lookup()
>
> How did you come doing this patch ? Any particular stack dump or
> something ?
>
no, no stack dump was found.
by reviewing code, I found it when I cooked another patch.

and the path that __ip_do_redirect() is called should be:

icmp_rcv |-> icmp_unreach  -\
              |-> icmp_redirect -> icmp_socket_deliver -> ipprot->err_handler()
[tcp_v4/6_err, sctp_v4/6_err, dccp_v4/6_err, udpv4/6_err ...]

-> ... -> __ip_do_redirect()

I checked the codes for many times, didn't find any rcu_lock on this path,
unless we treat BH as rcu_lock.

did I miss something ?

Thanks.

>
>
>
>
>

Re: [PATCH net] route: add rcu_read_lock when lookup route and update fnhe in __ip_do_redirect

[Eric Dumazet]

On Tue, 2016-02-02 at 01:16 +0800, Xin Long wrote:

> no, no stack dump was found.
> by reviewing code, I found it when I cooked another patch.
> 
> and the path that __ip_do_redirect() is called should be:
> 
> icmp_rcv |-> icmp_unreach  -\
>               |-> icmp_redirect -> icmp_socket_deliver -> ipprot->err_handler()
> [tcp_v4/6_err, sctp_v4/6_err, dccp_v4/6_err, udpv4/6_err ...]
> 
> -> ... -> __ip_do_redirect()
> 
> I checked the codes for many times, didn't find any rcu_lock on this path,
> unless we treat BH as rcu_lock.

Well I believe you missed something yes.

All RX paths run in rcu_read_lock()

ip_local_deliver_finish()
ipv6_rcv()
ip6_input_finish()

Re: [PATCH net] route: add rcu_read_lock when lookup route and update fnhe in __ip_do_redirect

[Xin Long]

On Tue, Feb 2, 2016 at 1:49 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2016-02-02 at 01:16 +0800, Xin Long wrote:
>
>> no, no stack dump was found.
>> by reviewing code, I found it when I cooked another patch.
>>
>> and the path that __ip_do_redirect() is called should be:
>>
>> icmp_rcv |-> icmp_unreach  -\
>>               |-> icmp_redirect -> icmp_socket_deliver -> ipprot->err_handler()
>> [tcp_v4/6_err, sctp_v4/6_err, dccp_v4/6_err, udpv4/6_err ...]
>>
>> -> ... -> __ip_do_redirect()
>>
>> I checked the codes for many times, didn't find any rcu_lock on this path,
>> unless we treat BH as rcu_lock.
>
> Well I believe you missed something yes.
>
> All RX paths run in rcu_read_lock()
>
> ip_local_deliver_finish()
> ipv6_rcv()
> ip6_input_finish()

Ah, right, I never thought the lock might start from ip_rcv.
Thanks, Eric.

>
>
>