Received: by 2002:a05:6358:4e97:b0:b3:742d:4702 with SMTP id ce23csp1865578rwb; Fri, 19 Aug 2022 10:40:39 -0700 (PDT) X-Google-Smtp-Source: AA6agR7phrI5hlBaWqH2ZJ6m3TxU3JsMh77QBb4x95KC8M7W+GPJ8fMuZ0RwWF4nhY5XeKofXk5I X-Received: by 2002:a63:1e5c:0:b0:41d:b225:9ee1 with SMTP id p28-20020a631e5c000000b0041db2259ee1mr6979580pgm.245.1660930838900; Fri, 19 Aug 2022 10:40:38 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1660930838; cv=none; d=google.com; s=arc-20160816; b=AzcRioSkIIv/ALPEmubea6yfjUa0iy9WBh+wlrE/LAqZ2W7OqNpJApfn5BwddTf6zb iGpEhz9j/eS0uUjiJVv25KObpQKHsBY9YDKuofZA+G2DJi4LwG1e+YYxp/+EwCbGyIoT vPzRBbBQO8lHcMccKpcenXqdaEvhusHzy8FW8Zi+eaKVq6DNxL5IDTL2O9xsjO4NCUeR rTzG+v5f0j8cuFHRUUKBVv+zxN/PWTSCWxW4c83aUivL6KHnkWaIpAwV1KYf451gYbzn SvpuqxpicNxGbj5E6g4yQ31smzXYNyllAHxvyR6KxdFx1YvZAudBiVxA/f6V6+I76RBV V2aw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:cc:to:subject :message-id:date:from:in-reply-to:references:mime-version :dkim-signature; bh=Fya4fZM01wtpV6jSWWXM/JoqlH2BGCdszj83hyqTE9Q=; b=JatBskneeekstCkPojxjGEQY1iFR/SYTuGcMcbFPOLEh/RDgXasLMIap8Al0V7Gees Xi5Sljjw3XS+HJRbPcb8qHUuZ0JMO9yY4iv5mB21tYsHHtXEyI9a2lg8qQ4sPQmAamGF CGEafsAGWA+8D31y8zH6X9Ri0BLaF8bL5AFOeaKBFn3qOh6aNXBTgxi5IXcwSleg7mdF VuVxiDR1odRufvUf2HOKHOuu1Um748tzIRQqLsjhtK6UfeHhVu07M7ZEJlR8zEF40U/q ew365EQpAiBhcZ0AjzcGuHEY1dobiHuOF7/dfzsno8JtWF+aDk3xiR7wlvCPuipqQHM0 Lmsg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b="a/OpAIk/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id a17-20020a63e411000000b00429cf3b6637si4453000pgi.697.2022.08.19.10.40.26; Fri, 19 Aug 2022 10:40:38 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b="a/OpAIk/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1354489AbiHSRVR (ORCPT + 99 others); Fri, 19 Aug 2022 13:21:17 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44338 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1352751AbiHSRVB (ORCPT ); Fri, 19 Aug 2022 13:21:01 -0400 Received: from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com [IPv6:2a00:1450:4864:20::52f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 89F4314D065; Fri, 19 Aug 2022 09:40:16 -0700 (PDT) Received: by mail-ed1-x52f.google.com with SMTP id c39so6341086edf.0; Fri, 19 Aug 2022 09:40:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc; bh=Fya4fZM01wtpV6jSWWXM/JoqlH2BGCdszj83hyqTE9Q=; b=a/OpAIk/kN1DVTd5wBYF+F7EgMXvFSvBuTaOGIBJxkeaU8wKI4dpl3ipvq1YyIb3A9 nrqBZAIr8x029+uxX0c19j1Bu7RPlwwbp0N+N9FYHNbYVjaULrG/wI91keaVSZs8dLeP UZJJBqZRaTln9uAFaklc0MmXBdPK254cAHipMS9AzpffDHST92WPwjkXWoV3t9GaHVhz U/U9RBBOJu+dEmuba0S/fliA3OQ00hZZtvsMWKreQ/WjRjjWBxJFWND5CvsmxkazTBdo oYg8NWCpRNn8Pe3SebfAYINWGdfJ66+PexCUqNYQl99Zk44ZdjhTgAalyi5mviVVcZ4H +Gfg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc; bh=Fya4fZM01wtpV6jSWWXM/JoqlH2BGCdszj83hyqTE9Q=; b=Uh3HyiW7BAU/wRVGlAYNEsGOxdAisHoSV3tSgthCPq2gp00tqzNMtffgzxC/UYI2FN Ws5Z2oxaIyuvcXAR3vrTvYbKBpojTRWhzENJ4vFpuDOrPJ5bSb0D6cAauQJ7k3zbH+Cf IeuymnOYAAzp1iGZ0FTqVGO2wW4iQ8WmRGi4MURYvLYFRTDk0KrXOYQi+Njkoh6RxcmX nXWm8Ger54AesXmWkCy8GVW2plH2p/oG0ETC1IAhaLdeRQI6bevUo4Po7PxMMYAwUnrx j+IpOhasNbQC/WVYzageMPWkn2es3hzByuqrMcBuGPFfXq1l6h4yJBMpl9aWZoVAhJMH +/6w== X-Gm-Message-State: ACgBeo37iW80cHRU8oKkCvfhgfudfI0RmxS7VcDW2aS/yHe0ryzKKWUa BsyFHKEeKiI0u8Oa7vWzaaRGM82GaefX+5dhyH4= X-Received: by 2002:a05:6402:298c:b0:446:a97:1800 with SMTP id eq12-20020a056402298c00b004460a971800mr6829163edb.421.1660927211176; Fri, 19 Aug 2022 09:40:11 -0700 (PDT) MIME-Version: 1.0 References: <87pmgxuy6v.fsf@toke.dk> <20220818221032.7b4lcpa7i4gchdvl@kashmir.localdomain> <87wnb4tmc0.fsf@toke.dk> In-Reply-To: <87wnb4tmc0.fsf@toke.dk> From: Alexei Starovoitov Date: Fri, 19 Aug 2022 09:39:59 -0700 Message-ID: Subject: Re: [PATCH bpf-next v2 3/4] bpf: Add support for writing to nf_conn:mark To: =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= Cc: Daniel Xu , bpf , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Kumar Kartikeya Dwivedi , Pablo Neira Ayuso , Florian Westphal , netfilter-devel , Network Development , LKML Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Aug 19, 2022 at 6:05 AM Toke H=C3=B8iland-J=C3=B8rgensen wrote: > > Daniel Xu writes: > > > Hi Toke, > > > > On Thu, Aug 18, 2022 at 09:52:08PM +0200, Toke H=C3=B8iland-J=C3=B8rgen= sen wrote: > >> Daniel Xu writes: > >> > >> > 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 > >> > metadata. > >> > >> Looking closer at the nf_conn definition, the mark field (and possibly > >> secmark) seems to be the only field that is likely to be feasible to > >> support direct writes to, as everything else either requires special > >> handling (like status and timeout), or they are composite field that > >> will require helpers anyway to use correctly. > >> > >> Which means we're in the process of creating an API where users have t= o > >> call helpers to fill in all fields *except* this one field that happen= s > >> to be directly writable. That seems like a really confusing and > >> inconsistent API, so IMO it strengthens the case for just making a > >> helper for this field as well, even though it adds a bit of overhead > >> (and then solving the overhead issue in a more generic way such as by > >> supporting clever inlining). > >> > >> -Toke > > > > I don't particularly have a strong opinion here. But to play devil's > > advocate: > > > > * It may be confusing now, but over time I expect to see more direct > > write support via BTF, especially b/c there is support for unstable > > helpers now. So perhaps in the future it will seem more sensible. > > Right, sure, for other structs. My point was that it doesn't look like > this particular one (nf_conn) is likely to grow any other members we can > access directly, so it'll be a weird one-off for that single field... > > > * The unstable helpers do not have external documentation. Nor should > > they in my opinion as their unstableness + stale docs may lead to > > undesirable outcomes. So users of the unstable API already have to > > splunk through kernel code and/or selftests to figure out how to wiel= d > > the APIs. All this to say there may not be an argument for > > discoverability. > > This I don't buy at all. Just because it's (supposedly) "unstable" is no They're unstable. Please don't start this 'but can we actually remove them' doubts. You're only confusing yourself and others. We tweaked kfuncs already. We removed tracepoints too after they were in a full kernel release. > excuse to design a bad API, or make it actively user-hostile by hiding 'bad API'? what? It's a direct field write. We do allow it in other data structures. > things so users have to go browse kernel code to know how to use it. So > in any case, we should definitely document everything. > > > * Direct writes are slightly more ergnomic than using a helper. > > This is true, and that's the main argument for doing it this way. The > point of my previous email was that since it's only a single field, > consistency weighs heavier than ergonomics :) I don't think the 'consistency' argument applies here. We already allow direct read of all fields. Also the field access is easier to handle with CO-RE.