From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 76B50C28B2B for ; Tue, 16 Aug 2022 11:29:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233873AbiHPL2b (ORCPT ); Tue, 16 Aug 2022 07:28:31 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48988 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234125AbiHPL2E (ORCPT ); Tue, 16 Aug 2022 07:28:04 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5ADAA2CDF7; Tue, 16 Aug 2022 03:43:37 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 03D99B8165D; Tue, 16 Aug 2022 10:43:36 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 86EA3C433C1; Tue, 16 Aug 2022 10:43:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1660646614; bh=yqe/EMQmD3VLnGHghypui0+k7E1d1JAMjbBdNpJp/Ac=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=WmDtk6Cf5S1czeeVK0DAKEntmyWn/ucxbgRUOJ7EvCnJ+hlnMmXKs7d2LzLusOWjG P9BZUnyFCygTSaBGyI+/ZMRsimDcw4lKZrmIl00p7YDKYDDZRS07+VkGYYjkDQNVZE /NHOq/5udUF8/K0YgcGNQGgSV+Ea36YQkWf7qRPhQgneFDWP9m6nB5Vly8RrTfkWTj Gx54EcrUu0WWbaHKs8uubbJDflr8MXwUMFtLvri3hNPrkYfanr9K/Ae48SXOeGfelg 0y7pVw8dF2stIh5VXYLv1fnS+4FQo+ZvQ2DKZC+sGfyc7TYLSt7uY6amkS3OtqU5wE xT3sWq5r6idFg== Received: by alrua-x1.borgediget.toke.dk (Postfix, from userid 1000) id 0ED1D55F5FC; Tue, 16 Aug 2022 12:43:32 +0200 (CEST) From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= To: Alexei Starovoitov , Florian Westphal Cc: Daniel Xu , bpf , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Kumar Kartikeya Dwivedi , Pablo Neira Ayuso , netfilter-devel , Network Development , LKML Subject: Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark In-Reply-To: References: X-Clacks-Overhead: GNU Terry Pratchett Date: Tue, 16 Aug 2022 12:43:32 +0200 Message-ID: <87v8qswjsb.fsf@toke.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Alexei Starovoitov writes: > On Mon, Aug 15, 2022 at 3:40 PM Florian Westphal wrote: >> >> Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> > > Support direct writes to nf_conn:mark from TC and XDP prog types. Th= is >> > > is useful when applications want to store per-connection metadata. T= his >> > > is also particularly useful for applications that run both bpf and >> > > iptables/nftables because the latter can trivially access this metad= ata. >> > > >> > > One example use case would be if a bpf prog is responsible for advan= ced >> > > packet classification and iptables/nftables is later used for routing >> > > due to pre-existing/legacy code. >> > > >> > > Signed-off-by: Daniel Xu >> > >> > 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_con= n? >> >> 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. > > +1 > I don't think we need to have a hard rule. > If fields is safe to access directly than it's faster > to let bpf prog read/write it. > There are no backward compat concerns. If conntrack side decides > to make that field special we can disallow direct writes in > the same kernel version. Right, I was under the impression we wanted all fields to be wrapper by helpers so that the struct owner could change their semantics without affecting users (and solve the performance issue by figuring out a generic way to inline those helpers). I guess there could also be an API consistency argument for doing this. However, I don't have a strong opinion on this, so if y'all prefer keeping these as direct field writes, that's OK with me. > These accesses, just like kfuncs, are unstable. Well, it will be interesting to see how that plays out the first time an application relying on one of these breaks on a kernel upgrade :) -Toke