| Message ID | 20180913203814.189698-1-posk@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net/ipv6: do not copy DST_NOCOUNT flag on rt init | expand |
On 9/13/18 1:38 PM, Peter Oskolkov wrote: > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > index 3eed045c65a5..a3902f805305 100644 > --- a/net/ipv6/route.c > +++ b/net/ipv6/route.c > @@ -946,7 +946,7 @@ static void ip6_rt_init_dst_reject(struct rt6_info *rt, struct fib6_info *ort) > > static void ip6_rt_init_dst(struct rt6_info *rt, struct fib6_info *ort) > { > - rt->dst.flags |= fib6_info_dst_flags(ort); > + rt->dst.flags |= fib6_info_dst_flags(ort) & ~DST_NOCOUNT; I think my mistake is setting dst.flags in ip6_rt_init_dst. Flags argument is passed to ip6_dst_alloc which is always invoked before ip6_rt_copy_init is called which is the only caller of ip6_rt_init_dst. > > if (ort->fib6_flags & RTF_REJECT) { > ip6_rt_init_dst_reject(rt, ort); >
On Thu, Sep 13, 2018 at 9:11 PM David Ahern <dsahern@gmail.com> wrote: > > On 9/13/18 1:38 PM, Peter Oskolkov wrote: > > > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > > index 3eed045c65a5..a3902f805305 100644 > > --- a/net/ipv6/route.c > > +++ b/net/ipv6/route.c > > @@ -946,7 +946,7 @@ static void ip6_rt_init_dst_reject(struct rt6_info *rt, struct fib6_info *ort) > > > > static void ip6_rt_init_dst(struct rt6_info *rt, struct fib6_info *ort) > > { > > - rt->dst.flags |= fib6_info_dst_flags(ort); > > + rt->dst.flags |= fib6_info_dst_flags(ort) & ~DST_NOCOUNT; > > I think my mistake is setting dst.flags in ip6_rt_init_dst. Flags > argument is passed to ip6_dst_alloc which is always invoked before > ip6_rt_copy_init is called which is the only caller of ip6_rt_init_dst. ip6_rt_cache_alloc calls ip6_dst_alloc with zero as flags; and only one flag is copied later (DST_HOST) outside of ip6_rt_init_dst(). If the flag assignment is completely removed from ip6_rt_init_dst(), then DST_NOPOLICY flag will be lost. Which may be OK, but is more than what this patch tries to solve (do not copy DST_NOCOUNT flag). > > > > > if (ort->fib6_flags & RTF_REJECT) { > > ip6_rt_init_dst_reject(rt, ort); > > >
On 9/17/18 9:11 AM, Peter Oskolkov wrote: > On Thu, Sep 13, 2018 at 9:11 PM David Ahern <dsahern@gmail.com> wrote: >> >> On 9/13/18 1:38 PM, Peter Oskolkov wrote: >> >>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c >>> index 3eed045c65a5..a3902f805305 100644 >>> --- a/net/ipv6/route.c >>> +++ b/net/ipv6/route.c >>> @@ -946,7 +946,7 @@ static void ip6_rt_init_dst_reject(struct rt6_info *rt, struct fib6_info *ort) >>> >>> static void ip6_rt_init_dst(struct rt6_info *rt, struct fib6_info *ort) >>> { >>> - rt->dst.flags |= fib6_info_dst_flags(ort); >>> + rt->dst.flags |= fib6_info_dst_flags(ort) & ~DST_NOCOUNT; >> >> I think my mistake is setting dst.flags in ip6_rt_init_dst. Flags >> argument is passed to ip6_dst_alloc which is always invoked before >> ip6_rt_copy_init is called which is the only caller of ip6_rt_init_dst. > > ip6_rt_cache_alloc calls ip6_dst_alloc with zero as flags; and only > one flag is copied later (DST_HOST) outside of ip6_rt_init_dst(). > If the flag assignment is completely removed from ip6_rt_init_dst(), > then DST_NOPOLICY flag will be lost. > > Which may be OK, but is more than what this patch tries to solve (do not > copy DST_NOCOUNT flag). After 5+ days mostly offline I just started looking at this problem. Give me some time to chase down a thought I had from my last response.
On 9/17/18 9:11 AM, Peter Oskolkov wrote: > On Thu, Sep 13, 2018 at 9:11 PM David Ahern <dsahern@gmail.com> wrote: >> >> On 9/13/18 1:38 PM, Peter Oskolkov wrote: >> >>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c >>> index 3eed045c65a5..a3902f805305 100644 >>> --- a/net/ipv6/route.c >>> +++ b/net/ipv6/route.c >>> @@ -946,7 +946,7 @@ static void ip6_rt_init_dst_reject(struct rt6_info *rt, struct fib6_info *ort) >>> >>> static void ip6_rt_init_dst(struct rt6_info *rt, struct fib6_info *ort) >>> { >>> - rt->dst.flags |= fib6_info_dst_flags(ort); >>> + rt->dst.flags |= fib6_info_dst_flags(ort) & ~DST_NOCOUNT; >> >> I think my mistake is setting dst.flags in ip6_rt_init_dst. Flags >> argument is passed to ip6_dst_alloc which is always invoked before >> ip6_rt_copy_init is called which is the only caller of ip6_rt_init_dst. > > ip6_rt_cache_alloc calls ip6_dst_alloc with zero as flags; and only > one flag is copied later (DST_HOST) outside of ip6_rt_init_dst(). > If the flag assignment is completely removed from ip6_rt_init_dst(), > then DST_NOPOLICY flag will be lost. > > Which may be OK, but is more than what this patch tries to solve (do not > copy DST_NOCOUNT flag). In the 4.17 kernel (prior to the fib6_info change), ip6_rt_cache_alloc calls __ip6_dst_alloc with 0 for flags so this is correct. The mistake is ip6_rt_copy_init -> ip6_rt_init_dst -> fib6_info_dst_flags. I believe the right fix is to drop the 'rt->dst.flags |= fib6_info_dst_flags(ort);' from ip6_rt_init_dst.
On Mon, Sep 17, 2018 at 9:59 AM David Ahern <dsahern@gmail.com> wrote: > > On 9/17/18 9:11 AM, Peter Oskolkov wrote: > > On Thu, Sep 13, 2018 at 9:11 PM David Ahern <dsahern@gmail.com> wrote: > >> > >> On 9/13/18 1:38 PM, Peter Oskolkov wrote: > >> > >>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c > >>> index 3eed045c65a5..a3902f805305 100644 > >>> --- a/net/ipv6/route.c > >>> +++ b/net/ipv6/route.c > >>> @@ -946,7 +946,7 @@ static void ip6_rt_init_dst_reject(struct rt6_info *rt, struct fib6_info *ort) > >>> > >>> static void ip6_rt_init_dst(struct rt6_info *rt, struct fib6_info *ort) > >>> { > >>> - rt->dst.flags |= fib6_info_dst_flags(ort); > >>> + rt->dst.flags |= fib6_info_dst_flags(ort) & ~DST_NOCOUNT; > >> > >> I think my mistake is setting dst.flags in ip6_rt_init_dst. Flags > >> argument is passed to ip6_dst_alloc which is always invoked before > >> ip6_rt_copy_init is called which is the only caller of ip6_rt_init_dst. > > > > ip6_rt_cache_alloc calls ip6_dst_alloc with zero as flags; and only > > one flag is copied later (DST_HOST) outside of ip6_rt_init_dst(). > > If the flag assignment is completely removed from ip6_rt_init_dst(), > > then DST_NOPOLICY flag will be lost. > > > > Which may be OK, but is more than what this patch tries to solve (do not > > copy DST_NOCOUNT flag). > > In the 4.17 kernel (prior to the fib6_info change), ip6_rt_cache_alloc > calls __ip6_dst_alloc with 0 for flags so this is correct. The mistake > is ip6_rt_copy_init -> ip6_rt_init_dst -> fib6_info_dst_flags. > > I believe the right fix is to drop the 'rt->dst.flags |= > fib6_info_dst_flags(ort);' from ip6_rt_init_dst. OK, I sent a v2 with the assignment removed. Thanks for the review!
diff --git a/net/ipv6/route.c b/net/ipv6/route.c index 3eed045c65a5..a3902f805305 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -946,7 +946,7 @@ static void ip6_rt_init_dst_reject(struct rt6_info *rt, struct fib6_info *ort) static void ip6_rt_init_dst(struct rt6_info *rt, struct fib6_info *ort) { - rt->dst.flags |= fib6_info_dst_flags(ort); + rt->dst.flags |= fib6_info_dst_flags(ort) & ~DST_NOCOUNT; if (ort->fib6_flags & RTF_REJECT) { ip6_rt_init_dst_reject(rt, ort);
DST_NOCOUNT in dst_entry::flags tracks whether the entry counts toward route cache size (net->ipv6.sysctl.ip6_rt_max_size). If the flag is NOT set, dst_ops::pcpuc_entries counter is incremented in dist_init() and decremented in dst_destroy(). This flag is tied to allocation/deallocation of dst_entry and should not be copied from another dst/route. Otherwise it can happen that dst_ops::pcpuc_entries counter grows until no new routes can be allocated because the counter reached ip6_rt_max_size due to DST_NOCOUNT not set and thus no counter decrements on gc-ed routes. Fixes: 3b6761d18bc1 ("net/ipv6: Move dst flags to booleans in fib entries") Cc: David Ahern <dsahern@gmail.com> Acked-by: Wei Wang <weiwan@google.com> Signed-off-by: Peter Oskolkov <posk@google.com> --- net/ipv6/route.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)