On 7/4/19 1:24 PM, David Miller wrote: > From: Ido Schimmel <ido...@idosch.org> > Date: Thu, 4 Jul 2019 19:26:38 +0300 > >> Both ip_neigh_gw4() and ip_neigh_gw6() can return either a valid pointer >> or an error pointer, but the code currently checks that the pointer is >> not NULL. > ... >> @@ -447,7 +447,7 @@ static struct neighbour *ipv4_neigh_lookup(const struct >> dst_entry *dst, >> n = ip_neigh_gw4(dev, pkey); >> } >> >> - if (n && !refcount_inc_not_zero(&n->refcnt)) >> + if (!IS_ERR(n) && !refcount_inc_not_zero(&n->refcnt)) >> n = NULL; >> >> rcu_read_unlock_bh(); > > Don't the callers expect only non-error pointers? > > All of this stuff is so confusing and fragile... >
The intention was to fold the lookup and neigh_create calls into a single helper. The lookup can return NULL if an entry does not exist; the create can return an ERR_PTR (variety of reasons in ___neigh_create). So the end result is that the new helper (lookup + create) can return a valid neigh entry or an ERR_PTR. When I converted ipv4_neigh_lookup and folded in the refcount bump, I missed updating the above check to account for ERR_PTR. Ido's patch looks correct to me. Thanks, Ido. Reviewed-by: David Ahern <dsah...@gmail.com>
