Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i6a694271:Fastmail
Date:   Tue, 18 Jul 2023 15:45:32 -0600
From:   Daniel Xu <dxu@dxuuu.xyz>
To:     Florian Westphal <fw@strlen.de>
Cc:     Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Pablo Neira Ayuso <pablo@netfilter.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Jozsef Kadlecsik <kadlec@netfilter.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        bpf <bpf@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        netfilter-devel <netfilter-devel@vger.kernel.org>,
        coreteam@netfilter.org,
        Network Development <netdev@vger.kernel.org>,
        David Ahern <dsahern@kernel.org>
Subject: Re: [PATCH bpf-next v4 2/6] netfilter: bpf: Support
 BPF_F_NETFILTER_IP_DEFRAG in netfilter link
Message-ID: <6av46ydgbufp5x23lempwmutcsjuy6efpysbvnqxjoirng43tr@gcyqxhln2x6f>
References: <cover.1689203090.git.dxu@dxuuu.xyz>
 <d3b0ff95c58356192ea3b50824f8cdbf02c354e3.1689203090.git.dxu@dxuuu.xyz>
 <CAADnVQKKfEtZYZxihxvG3aQ34E1m95qTZ=jTD7yd0qvOASpAjQ@mail.gmail.com>
 <kwiwaeaijj6sxwz5fhtxyoquhz2kpujbsbeajysufgmdjgyx5c@f6lqrd23xr5f>
 <CAADnVQLcAoN5z+HD_44UKgJJc6t5TPW8+Ai9We0qJpau4NtEzA@mail.gmail.com>
 <wltfmammaf5g4gumsbna4kmwo6dtd24g472o7kgkug42dhwcy2@32fmd7q6kvg4>
 <CAADnVQJQZ2jQSWByVvi3N2ZOoL0XDSJzx5biSVvq=inS7OSW7A@mail.gmail.com>
 <t6wypww537golmoosbikfuombrqq555fh5mbycwl4whto6joo4@hcqlospkgqyr>
 <20230714094741.GA7912@breakpoint.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230714094741.GA7912@breakpoint.cc>
Precedence: bulk

Hi Florian,

On Fri, Jul 14, 2023 at 11:47:41AM +0200, Florian Westphal wrote:
> Daniel Xu <dxu@dxuuu.xyz> wrote:
> > On Thu, Jul 13, 2023 at 04:10:03PM -0700, Alexei Starovoitov wrote:
> > > Why is rcu_assign_pointer() used?
> > > If it's not RCU protected, what is the point of rcu_*() accessors
> > > and rcu_read_lock() ?
> > > 
> > > In general, the pattern:
> > > rcu_read_lock();
> > > ptr = rcu_dereference(...);
> > > rcu_read_unlock();
> > > ptr->..
> > > is a bug. 100%.
> 
> FWIW, I agree with Alexei, it does look... dodgy.
> 
> > The reason I left it like this is b/c otherwise I think there is a race
> > with module unload and taking a refcnt. For example:
> > 
> > ptr = READ_ONCE(global_var)
> >                                              <module unload on other cpu>
> > // ptr invalid
> > try_module_get(ptr->owner) 
> >
> 
> Yes, I agree.
> 
> > I think the the synchronize_rcu() call in
> > kernel/module/main.c:free_module() protects against that race based on
> > my reading.
> > 
> > Maybe the ->enable() path can store a copy of the hook ptr in
> > struct bpf_nf_link to get rid of the odd rcu_dereference()?
> > 
> > Open to other ideas too -- would appreciate any hints.
> 
> I would suggest the following:
> 
> - Switch ordering of patches 2 and 3.
>   What is currently patch 3 would add the .owner fields only.
> 
> Then, what is currently patch #2 would document the rcu/modref
> interaction like this (omitting error checking for brevity):
> 
> rcu_read_lock();
> v6_hook = rcu_dereference(nf_defrag_v6_hook);
> if (!v6_hook) {
>         rcu_read_unlock();
>         err = request_module("nf_defrag_ipv6");
>         if (err)
>                  return err < 0 ? err : -EINVAL;
>         rcu_read_lock();
> 	v6_hook = rcu_dereference(nf_defrag_v6_hook);
> }
> 
> if (v6_hook && try_module_get(v6_hook->owner))
> 	v6_hook = rcu_pointer_handoff(v6_hook);
> else
> 	v6_hook = NULL;
> 
> rcu_read_unlock();
> 
> if (!v6_hook)
> 	err();
> v6_hook->enable();
> 
> 
> I'd store the v4/6_hook pointer in the nf bpf link struct, its probably more
> self-explanatory for the disable side in that we did pick up a module reference
> that we still own at delete time, without need for any rcu involvement.
> 
> Because above handoff is repetitive for ipv4 and ipv6,
> I suggest to add an agnostic helper for this.
> 
> I know you added distinct structures for ipv4 and ipv6 but if they would use
>  the same one you could add
> 
> static const struct nf_defrag_hook *get_proto_frag_hook(const struct nf_defrag_hook __rcu *hook,
> 							const char *modulename);
> 
> And then use it like:
> 
> v4_hook = get_proto_frag_hook(nf_defrag_v4_hook, "nf_defrag_ipv4");
> 
> Without a need to copy the modprobe and handoff part.
> 
> What do you think?

That sounds reasonable to me. I'll give it a shot. Thanks for the input!

Daniel