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;
Date:   Wed, 17 Aug 2022 15:05:01 -0700
From:   Martin KaFai Lau <kafai@fb.com>
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     Daniel Xu <dxu@dxuuu.xyz>, Martin KaFai Lau <martin.lau@linux.dev>,
        bpf <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 Neira Ayuso <pablo@netfilter.org>,
        Florian Westphal <fw@strlen.de>,
        Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@kernel.org>,
        netfilter-devel <netfilter-devel@vger.kernel.org>,
        Network Development <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH bpf-next v2 3/4] bpf: Add support for writing to
 nf_conn:mark
Message-ID: <20220817220501.kiftkkaqepooforu@kafai-mbp>
References: <cover.1660761470.git.dxu@dxuuu.xyz>
 <edbca42217a73161903a50ba07ec63c5fa5fde00.1660761470.git.dxu@dxuuu.xyz>
 <CAADnVQ+G0Hju-OeN6e=JLPQzODxGXCsP7OuVbex1y-EYr6Z5Yw@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAADnVQ+G0Hju-OeN6e=JLPQzODxGXCsP7OuVbex1y-EYr6Z5Yw@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?hS9txzFm+Dx04gU1OCtCjIcGtP4SHf5xIC+BlzflUqu6f5vw9gsiMBWDRqju?=
 =?us-ascii?Q?8HPIDeBVbnG2JQ1soshUAIKtSXptuyPZ0wbPRBfKdXFYEVefrVKKC8NKQvBs?=
 =?us-ascii?Q?ZYrPfoL+tyn526YEc906biAaR9DB9lbD2WQ+ilnUOTVammsybUz8uL/mVmH+?=
 =?us-ascii?Q?Mi84t/Nrs4hu5obD3piceQjQfowid74+IB5oXGrMjnsHOB/Cy5EiGSD69kGP?=
 =?us-ascii?Q?FtJF4SQPaXF4Fr+HanviQiL1cCxJOUUi5VwQjjEjv25m4RMpFk/moI9VsTqW?=
 =?us-ascii?Q?X5HLUncu4v05ZNvqA5uYM54W3TMz2SZ24aRnad8skPxwMxRs+9ZrWZCxjHgo?=
 =?us-ascii?Q?Rai6gtPBicccYgzOdGrFEw+5FktMvPh59/gCspH8HOZHDY8y4R3OEeZb/Sie?=
 =?us-ascii?Q?bAlL1U58htnnd6j6TaOW8DDzBJczM85/URU/n4s6TwC8Hd+3bEfxy+59dAlU?=
 =?us-ascii?Q?yjJb4QtsHcnPteIikg4bLFPFTKOW+QCfj0MdUfatAHTH/K2rC3teBGSPs13h?=
 =?us-ascii?Q?U9aEUtUiI19wpI0A3GaPcgWd+WN+7lSndJAT9x/anzNFHDR7sk+1jBNuVrqd?=
 =?us-ascii?Q?kYQrHLaoBqzj7yGYxrjctUeXfOS3r55xaq+24Kb2REIrwJIItr9NorvtiL8H?=
 =?us-ascii?Q?b89vZQROVnbFitVFSCDMYdVm87pIpkZi6SI6reexlHlC5uEbkEVv8sp3V9Yl?=
 =?us-ascii?Q?XEXDmh5LxBT5/gBRQS/2/qy7kg02mmqefuCftn5lIOcApyDuiG8E5YfvAsRc?=
 =?us-ascii?Q?L5o7YAixICAK7zhSgsEtPIeLCHQ49aVXyXfnstl748QgseBhD+xTsa28W8f6?=
 =?us-ascii?Q?PiAZVGz1A7cc/CcRKaoZpcyw8R+auaa5+KYbcEQqT8KF0wYazyASvic9e5/U?=
 =?us-ascii?Q?33aCHsOtV9j3lnPl0om8+jllaHBc6gXvjnkFhZz1zvD4ix046B6kr3Y6FKw+?=
 =?us-ascii?Q?AnPpiCG23X9276lS6krqANpNItRpQ4sNeBs0nH8kw1bDditOZ1VhLt+hfAYP?=
 =?us-ascii?Q?me16Tb0zACYs2G9otyJPGLwB4prkezpxpS4peWzNHOFF7bNAv0OBL1RJUAaQ?=
 =?us-ascii?Q?v91GVtEAQ2CJa2iswvN7Xgz/ZcmZJStLNP+fiX+61ct1NEYKp9wmOOg7m14k?=
 =?us-ascii?Q?OfVeo0cQiKOHD/8HH+3D3muhVflusOy6tD61GvtuVruGxmDNRMobUqmwaWn3?=
 =?us-ascii?Q?w16jqltXZbkVWGxAoCIobyn3TFFEnu39Mq2MBNSVaufJHC8rNaKE4EE+g+am?=
 =?us-ascii?Q?noR7wmq6l9+zzDv5SSYpnrRcvwkad6sRCb5niPRGTooAKWk3eYkWO32sK9PE?=
 =?us-ascii?Q?eMGkMJbjuACWHzAgDeb3+euTU8KQ6kpX6CsUcr3s8m6K7kCIYHIh1AdFoXfk?=
 =?us-ascii?Q?xvNufxN7sAG4GJ1gmcUb5VIgsueecMGbv1HknxGAI0OyOPIuKwkwlsNRhn42?=
 =?us-ascii?Q?PxTk3ZrKHP6L8LOtk6geVRSbbXttrQv70r6+iff0t6nTapCe9ZUVacENMMJy?=
 =?us-ascii?Q?oJCyRcuc65Mb+zq3bvPW7SjdZ2QQkHgYw4QuAJHLWLwDrs2iTksK68aOgA/o?=
 =?us-ascii?Q?kERaA5t1jj526dGHXUN8kcBv/bO8vudKYXdlhyFgepnLn/QUj80IOo35dJQt?=
 =?us-ascii?Q?UA=3D=3D?=
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fa6aabaf-fbcf-4267-55f2-08da809c894f
X-MS-Exchange-CrossTenant-AuthSource: MW4PR15MB4475.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Aug 2022 22:05:03.4527
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: IjtyhyWPDolqkTUU7KLeGySuapeG+i/Fx3CJPvNOdWOlSZ+tYFmsEjQJiYmEFSl8
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR15MB5246
X-Proofpoint-GUID: fgDZipQJ5COrgVhGF6VnQZXhO3uq3tSE
X-Proofpoint-ORIG-GUID: fgDZipQJ5COrgVhGF6VnQZXhO3uq3tSE
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-08-17_15,2022-08-16_02,2022-06-22_01
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED 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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Aug 17, 2022 at 02:30:01PM -0700, Alexei Starovoitov wrote:
> On Wed, Aug 17, 2022 at 11:43 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
> >
> > +/* Check writes into `struct nf_conn` */
> > +int nf_conntrack_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)
> > +{
> > +       const struct btf_type *nct = READ_ONCE(nf_conn_type);
> > +       s32 type_id;
> > +       size_t end;
> > +
> > +       if (!nct) {
> > +               type_id = btf_find_by_name_kind(btf, "nf_conn", BTF_KIND_STRUCT);
> > +               if (type_id < 0)
> > +                       return -EINVAL;
> > +
> > +               nct = btf_type_by_id(btf, type_id);
> > +               WRITE_ONCE(nf_conn_type, nct);
> > +       }
> > +
> > +       if (t != nct) {
> > +               bpf_log(log, "only read is supported\n");
> > +               return -EACCES;
> > +       }
> > +
> > +       switch (off) {
> > +#if defined(CONFIG_NF_CONNTRACK_MARK)
> > +       case offsetof(struct nf_conn, mark):
> > +               end = offsetofend(struct nf_conn, mark);
> > +               break;
> > +#endif
> > +       default:
> > +               bpf_log(log, "no write support to nf_conn at off %d\n", off);
> > +               return -EACCES;
> > +       }
> > +
> > +       if (off + size > end) {
> > +               bpf_log(log,
> > +                       "write access at off %d with size %d beyond the member of nf_conn ended at %zu\n",
> > +                       off, size, end);
> > +               return -EACCES;
> > +       }
> > +
> > +       return NOT_INIT;
> 
> Took me a long time to realize that this is a copy-paste
> from net/ipv4/bpf_tcp_ca.c.
> It's not wrong, but misleading.
> When atype == BPF_READ the return value from
> btf_struct_access should only be error<0, SCALAR_VALUE, PTR_TO_BTF_ID.
> For atype == BPF_WRITE we should probably standardize on
> error<0, or 0.
> 
> The NOT_INIT happens to be zero, but explicit 0
> is cleaner to avoid confusion that this is somehow enum bpf_reg_type.
> 
> Martin,
> since you've added this code in bpf_tcp_ca, wdyt?
Yep, sgtm.  This will be less confusing.