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:   Fri, 19 Aug 2022 18:21:24 -0600
From:   Daniel Xu <dxu@dxuuu.xyz>
To:     Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc:     bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
        andrii@kernel.org, pablo@netfilter.org, fw@strlen.de,
        toke@kernel.org, martin.lau@linux.dev,
        netfilter-devel@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf-next v3 4/5] bpf: Add support for writing to
 nf_conn:mark
Message-ID: <20220820002124.g3d5fud5klvrkjil@kashmir.localdomain>
References: <cover.1660951028.git.dxu@dxuuu.xyz>
 <f44b2eebe48f0653949f59c5bcf23af029490692.1660951028.git.dxu@dxuuu.xyz>
 <CAP01T74fSh6Z=54O+ORKJD7i_izb7rUe3-mHKLgRdrckcisvkw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAP01T74fSh6Z=54O+ORKJD7i_izb7rUe3-mHKLgRdrckcisvkw@mail.gmail.com>
Precedence: bulk

Hi Kumar,

On Sat, Aug 20, 2022 at 01:46:04AM +0200, Kumar Kartikeya Dwivedi wrote:
> On Sat, 20 Aug 2022 at 01:23, Daniel Xu <dxu@dxuuu.xyz> wrote:
[...]
> > +static int tc_cls_act_btf_struct_access(struct bpf_verifier_log *log,
> > +                                       const struct btf *btf,
> > +                                       const struct btf_type *t, int off,
> > +                                       int size, enum bpf_access_type atype,
> > +                                       u32 *next_btf_id,
> > +                                       enum bpf_type_flag *flag)
> > +{
> > +       btf_struct_access_t sa;
> > +
> > +       if (atype == BPF_READ)
> > +               return btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
> > +                                        flag);
> > +
> > +       sa = READ_ONCE(nf_conntrack_btf_struct_access);
> 
> This looks unsafe. How do you prevent this race?
> 
> CPU 0                                              CPU 1
> sa = READ_ONCE(nf_ct_bsa);
> 
> delete_module("nf_conntrack", ..);
> 
> WRITE_ONCE(nf_ct_bsa, NULL);
>                                                          // finishes
> successfully
> if (sa)
>     return sa(...); // oops
> 
> i.e. what keeps the module alive while we execute its callback?
> 
> Using a mutex is one way (as I suggested previously), either you
> acquire it before unload, or after. If after, you see cb as NULL,
> otherwise if unload is triggered concurrently it waits to acquire the
> mutex held by us. Unsetting the cb would be the first thing the module
> would do.
> 
> You can also hold a module reference, but then you must verify it is
> nf_conntrack's BTF before using btf_try_get_module.
> But _something_ needs to be done to prevent the module from going away
> while we execute its code.

I think I somehow convinced myself that nf_conntrack_core.o is always
compiled in. Due to some of the garbage collection semantics I saw in
the code.

Lemme take a closer look (for learning I guess). Mutex is probably
safest bet.

[...]

Thanks,
Daniel