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
User-Agent: Cyrus-JMAP/3.7.0-alpha0-841-g7899e99a45-fm-20220811.002-g7899e99a
Mime-Version: 1.0
Message-Id: <668406b9-714f-4ade-889d-051cf42ceefc@www.fastmail.com>
In-Reply-To: <20220817183453.GA24008@breakpoint.cc>
References: <cover.1660592020.git.dxu@dxuuu.xyz>
 <f850bb7e20950736d9175c61d7e0691098e06182.1660592020.git.dxu@dxuuu.xyz>
 <871qth87r1.fsf@toke.dk> <20220815224011.GA9821@breakpoint.cc>
 <5c7ac2ab-942f-4ee7-8a9c-39948a40681c@www.fastmail.com>
 <20220817183453.GA24008@breakpoint.cc>
Date:   Wed, 17 Aug 2022 12:41:12 -0600
From:   "Daniel Xu" <dxu@dxuuu.xyz>
To:     "Florian Westphal" <fw@strlen.de>
Cc:     =?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@kernel.org>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        "Alexei Starovoitov" <ast@kernel.org>,
        "Daniel Borkmann" <daniel@iogearbox.net>,
        "Andrii Nakryiko" <andrii@kernel.org>,
        "Kumar Kartikeya Dwivedi" <memxor@gmail.com>, pablo@netfilter.org,
        netfilter-devel@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Aug 17, 2022, at 12:34 PM, Florian Westphal wrote:
> Daniel Xu <dxu@dxuuu.xyz> wrote:
>> On Mon, Aug 15, 2022, at 4:40 PM, Florian Westphal wrote:
>> > Toke H=C3=B8iland-J=C3=B8rgensen <toke@kernel.org> wrote:
>> >> > Support direct writes to nf_conn:mark from TC and XDP prog types=
. This
>> >> > is useful when applications want to store per-connection metadat=
a. This
>> >> > is also particularly useful for applications that run both bpf a=
nd
>> >> > iptables/nftables because the latter can trivially access this m=
etadata.
>> >> >
>> >> > One example use case would be if a bpf prog is responsible for a=
dvanced
>> >> > packet classification and iptables/nftables is later used for ro=
uting
>> >> > due to pre-existing/legacy code.
>> >> >
>> >> > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
>> >>=20
>> >> Didn't we agree the last time around that all field access should =
be
>> >> using helper kfuncs instead of allowing direct writes to struct nf=
_conn?
>> >
>> > I don't see why ct->mark needs special handling.
>> >
>> > It might be possible we need to change accesses on nf/tc side to use
>> > READ/WRITE_ONCE though.
>>=20
>> I reviewed some of the LKMM literature and I would concur that
>> READ/WRITE_ONCE() is necessary. Especially after this patchset.
>>=20
>> However, it's unclear to me if this is a latent issue. IOW: is reading
>> ct->mark protected by a lock? I only briefly looked but it doesn't
>> seem like it.
>
> No, its not protected by a lock.  READ/WRITE_ONCE is unrelated to your
> patchset, this is a pre-existing "bug".

Thanks for confirming. Since it's pre-existing I will send out a followup
patchset then.

Thanks,
Daniel