| Message ID | 20231208194523.312416-2-thinker.li@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix dangling pointer at f6i->gc_link. | expand |
On 12/8/23 12:45 PM, thinker.li@gmail.com wrote: > From: Kui-Feng Lee <thinker.li@gmail.com> > > Check f6i->fib6_node and hlist_unhashed(&f6i->gc_link) before inserting a > f6i (fib6_info) to tb6_gc_hlist. > > The current implementation checks if f6i->fib6_table is not NULL to > determines if a f6i is on a tree, however it is not enough. When a f6i is > removed from a fib6_table, f6i->fib6_table is not reset. However, fib6_node > is always reset when a f6i is removed from a fib6_table and is set when a > f6i is added to a fib6_table. So, f6i->fib6_node is a reliable way to > determine if a f6i is on a tree. Which is an indication that the table is not the right check but neither is the fib6_node. If expires is set on a route entry, add it to the gc_list; if expires is reset on a route entry, remove it from the gc_list. If the value of expires is changed while on the gc_list list, just update the expires value.
On 12/8/23 14:43, David Ahern wrote: > On 12/8/23 12:45 PM, thinker.li@gmail.com wrote: >> From: Kui-Feng Lee <thinker.li@gmail.com> >> >> Check f6i->fib6_node and hlist_unhashed(&f6i->gc_link) before inserting a >> f6i (fib6_info) to tb6_gc_hlist. >> >> The current implementation checks if f6i->fib6_table is not NULL to >> determines if a f6i is on a tree, however it is not enough. When a f6i is >> removed from a fib6_table, f6i->fib6_table is not reset. However, fib6_node >> is always reset when a f6i is removed from a fib6_table and is set when a >> f6i is added to a fib6_table. So, f6i->fib6_node is a reliable way to >> determine if a f6i is on a tree. > > Which is an indication that the table is not the right check but neither > is the fib6_node. If expires is set on a route entry, add it to the > gc_list; if expires is reset on a route entry, remove it from the > gc_list. If the value of expires is changed while on the gc_list list, > just update the expires value. > I don't quite follow you. If an entry is not on a tree, why do we still add the entry to the gc list? (This is the reason to check f6i->fib6_node.) The changes in this patch rely on two indications, 1. if a f6i is on a tree, 2. if a f6i is on a gc list (described in the 3rd paragraph of the comment log.) An entry is added to a gc list only if it has expires and is not on the list yet. An entry is removed from a gc list only if it is on a gc list. And, just like what you said earlier, it just updates the expires value while an entry is already on a gc list.
On 12/8/23 15:19, Kui-Feng Lee wrote: > > > On 12/8/23 14:43, David Ahern wrote: >> On 12/8/23 12:45 PM, thinker.li@gmail.com wrote: >>> From: Kui-Feng Lee <thinker.li@gmail.com> >>> >>> Check f6i->fib6_node and hlist_unhashed(&f6i->gc_link) before >>> inserting a >>> f6i (fib6_info) to tb6_gc_hlist. >>> >>> The current implementation checks if f6i->fib6_table is not NULL to >>> determines if a f6i is on a tree, however it is not enough. When a >>> f6i is >>> removed from a fib6_table, f6i->fib6_table is not reset. However, >>> fib6_node >>> is always reset when a f6i is removed from a fib6_table and is set >>> when a >>> f6i is added to a fib6_table. So, f6i->fib6_node is a reliable way to >>> determine if a f6i is on a tree. >> >> Which is an indication that the table is not the right check but neither >> is the fib6_node. If expires is set on a route entry, add it to the >> gc_list; if expires is reset on a route entry, remove it from the >> gc_list. If the value of expires is changed while on the gc_list list, >> just update the expires value. >> > > I don't quite follow you. > If an entry is not on a tree, why do we still add the entry to the gc > list? (This is the reason to check f6i->fib6_node.) > > The changes in this patch rely on two indications, 1. if a f6i is on a > tree, 2. if a f6i is on a gc list (described in the 3rd paragraph of > the comment log.) > An entry is added to a gc list only if it has expires and is not on the > list yet. An entry is removed from a gc list only if it is on a gc list. > And, just like what you said earlier, it just updates the expires value > while an entry is already on a gc list. Add more context here. Use rt6_route_rcv() as an example. It looks up or adds a route first. Then it changes expires of the route at the very end of the function. There is gap between looking up and the modification. The f6i found here can be removed from the tree in-between. If the f6i here is added to the gc list even it has been removed from the tree, fib6_info_release() at the end of rt6_route_rcv() might free the f6i while the f6i is still in the gc list. This is why it checks if a f6i is on the tree with the protection of tb6_lock.
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h index 1ba9f4ddf2f6..1213722c394f 100644 --- a/include/net/ip6_fib.h +++ b/include/net/ip6_fib.h @@ -500,21 +500,47 @@ void fib6_gc_cleanup(void); int fib6_init(void); -/* fib6_info must be locked by the caller, and fib6_info->fib6_table can be - * NULL. - */ -static inline void fib6_set_expires_locked(struct fib6_info *f6i, - unsigned long expires) +static inline void fib6_add_gc_list(struct fib6_info *f6i) { struct fib6_table *tb6; tb6 = f6i->fib6_table; - f6i->expires = expires; - if (tb6 && !fib6_has_expires(f6i)) + if (tb6 && + rcu_dereference_protected(f6i->fib6_node, + lockdep_is_held(&tb6->tb6_lock)) && + hlist_unhashed(&f6i->gc_link)) hlist_add_head(&f6i->gc_link, &tb6->tb6_gc_hlist); +} + +static inline void fib6_del_gc_list(struct fib6_info *f6i) +{ + if (!hlist_unhashed(&f6i->gc_link)) + hlist_del_init(&f6i->gc_link); +} + +static inline void __fib6_set_expires(struct fib6_info *f6i, + unsigned long expires) +{ + f6i->expires = expires; f6i->fib6_flags |= RTF_EXPIRES; } +static inline void __fib6_clean_expires(struct fib6_info *f6i) +{ + f6i->fib6_flags &= ~RTF_EXPIRES; + f6i->expires = 0; +} + +/* fib6_info must be locked by the caller, and fib6_info->fib6_table can be + * NULL. + */ +static inline void fib6_set_expires_locked(struct fib6_info *f6i, + unsigned long expires) +{ + __fib6_set_expires(f6i, expires); + fib6_add_gc_list(f6i); +} + /* fib6_info must be locked by the caller, and fib6_info->fib6_table can be * NULL. If fib6_table is NULL, the fib6_info will no be inserted into the * list of GC candidates until it is inserted into a table. @@ -529,10 +555,8 @@ static inline void fib6_set_expires(struct fib6_info *f6i, static inline void fib6_clean_expires_locked(struct fib6_info *f6i) { - if (fib6_has_expires(f6i)) - hlist_del_init(&f6i->gc_link); - f6i->fib6_flags &= ~RTF_EXPIRES; - f6i->expires = 0; + fib6_del_gc_list(f6i); + __fib6_clean_expires(f6i); } static inline void fib6_clean_expires(struct fib6_info *f6i) diff --git a/net/ipv6/route.c b/net/ipv6/route.c index b132feae3393..dcaeb88d73aa 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -3763,10 +3763,10 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg, rt->dst_nocount = true; if (cfg->fc_flags & RTF_EXPIRES) - fib6_set_expires_locked(rt, jiffies + - clock_t_to_jiffies(cfg->fc_expires)); + __fib6_set_expires(rt, jiffies + + clock_t_to_jiffies(cfg->fc_expires)); else - fib6_clean_expires_locked(rt); + __fib6_clean_expires(rt); if (cfg->fc_protocol == RTPROT_UNSPEC) cfg->fc_protocol = RTPROT_BOOT;