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: <5c7ac2ab-942f-4ee7-8a9c-39948a40681c@www.fastmail.com>
In-Reply-To: <20220815224011.GA9821@breakpoint.cc>
References: <cover.1660592020.git.dxu@dxuuu.xyz>
 <f850bb7e20950736d9175c61d7e0691098e06182.1660592020.git.dxu@dxuuu.xyz>
 <871qth87r1.fsf@toke.dk> <20220815224011.GA9821@breakpoint.cc>
Date:   Wed, 17 Aug 2022 12:28:55 -0600
From:   "Daniel Xu" <dxu@dxuuu.xyz>
To:     "Florian Westphal" <fw@strlen.de>,
        =?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@kernel.org>
Cc:     "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

Hi Florian,

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. T=
his
>> > is useful when applications want to store per-connection metadata. =
This
>> > is also particularly useful for applications that run both bpf and
>> > iptables/nftables because the latter can trivially access this meta=
data.
>> >
>> > One example use case would be if a bpf prog is responsible for adva=
nced
>> > packet classification and iptables/nftables is later used for routi=
ng
>> > 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_co=
nn?
>
> 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.

I reviewed some of the LKMM literature and I would concur that
READ/WRITE_ONCE() is necessary. Especially after this patchset.

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.

I'll do some more digging.

In the meantime, I'll send out a v2 on this patchset and I'll plan on
sending out a followup patchset for adding READ/WRITE_ONCE()
to ct->mark accesses.

Thanks,
Daniel