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:   Mon, 15 May 2023 19:08:39 +0200
From:   Larysa Zaremba <larysa.zaremba@intel.com>
To:     Jesper Dangaard Brouer <jbrouer@redhat.com>
CC:     <bpf@vger.kernel.org>, <brouer@redhat.com>,
        Stanislav Fomichev <sdf@google.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
        Jesse Brandeburg <jesse.brandeburg@intel.com>,
        "Tony Nguyen" <anthony.l.nguyen@intel.com>,
        Anatoly Burakov <anatoly.burakov@intel.com>,
        Alexander Lobakin <alexandr.lobakin@intel.com>,
        Magnus Karlsson <magnus.karlsson@gmail.com>,
        Maryam Tahhan <mtahhan@redhat.com>,
        <xdp-hints@xdp-project.net>, <netdev@vger.kernel.org>,
        <intel-wired-lan@lists.osuosl.org>, <linux-kernel@vger.kernel.org>,
        Aleksander Lobakin <aleksander.lobakin@intel.com>
Subject: Re: [PATCH RESEND bpf-next 14/15] net, xdp: allow metadata > 32
Message-ID: <ZGJnFxzDTV2qE4zZ@lincoln>
References: <20230512152607.992209-1-larysa.zaremba@intel.com>
 <20230512152607.992209-15-larysa.zaremba@intel.com>
 <ee1ad4f2-34ab-4377-14d5-532cb0687180@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ee1ad4f2-34ab-4377-14d5-532cb0687180@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?uS4XK+t6oC2AFnBTvftRNvuU1+U8Z2Z8roT6SFjpNHi314fvtaKgSw5YrMYo?=
 =?us-ascii?Q?gx23swfIYWN6aClZgQZeCpCudYipFKoulZvnw7ze9ZWO4IOy/Q9xTrYUoOFD?=
 =?us-ascii?Q?Sbz/e8ye8N4LmsAujbUidm4D4e7XubnkKOV5eKJbp3XiGCqDIC2kFf0YNn6o?=
 =?us-ascii?Q?ZBrsB7PObglP1WQ6Der0DS7YJ81zdDTHsxAlx9yk7u9wyMHnup8e4+1WMtEk?=
 =?us-ascii?Q?vO6GS9G2mGYTI8WZm8uexLxlfYZqAZ+AidG5NBzF7YQMJtY2qKf1GbkQ5owI?=
 =?us-ascii?Q?DX6dXjQnG6yxjytH+8ib7hY0IAbt8JjW0uJAdrFBUVBc5IzfN82Pl/8zBbsO?=
 =?us-ascii?Q?f7HCgAacPUxJVvMuvDxjzns/eLlh2TP3zsEFuOnG4OLrGbm1FS4mkAU0Di0i?=
 =?us-ascii?Q?ZuJhx7EPm+MLTX2VNDdi3xz1IeqTMhUe/CPX9mIZJXTyO28Ek5Q2mNwhuelX?=
 =?us-ascii?Q?WIlPwBMHdXX8VN8VzbvViTVYR9sRunxeHe4dvTTDkt3oQwqPc0UHe4LR56B4?=
 =?us-ascii?Q?FIhVR6eY0Ye80TOSfA3Y7In4pxcW4R8mUsnVBUC/A4z8XWqzmUZea4saEWjr?=
 =?us-ascii?Q?hw5yWbuRCZiOFFWoNAKXiRHZU7TanTawGjhaZH+JrSD2rcTFhGh6Yc96nAZz?=
 =?us-ascii?Q?Ng/QBd3DYQ4D58BC2+bRiqJNfXtbXbRUwetU4rWlCZS5OizIJw9ailLFuUdI?=
 =?us-ascii?Q?qcKJK4A9uqELyAH6sA+2ydOpUuS5YSKqlSRf/VoGXLrqsLVCA4dePjoVdH6o?=
 =?us-ascii?Q?+vnKDeVGkMPxzgzvbL2mmTONjKRUpV8vLqvxlfLq5hAl4xG/qW1irG7Nrtt0?=
 =?us-ascii?Q?8iBfOCI8Ni3/5XkX7sRO64LhIabqeXN2yGY0uFnNyEC9GWQm5hoAqihpF3jz?=
 =?us-ascii?Q?4E8C+c3eKSsQt8LNy8evbWo1qaZjdW4Fw/uT7DvKAl3V0sm/qFil9gXXFnbB?=
 =?us-ascii?Q?xewGvWh33V9/5xFA8L3TlNPsiu5aEhR0yUSnEjwff+EWnpyREiiEXixzSGZf?=
 =?us-ascii?Q?xGVjeL1OPl1w7UqxwO2cQ5yOtE7aBodpVCrNDMywS23rHeI9R08hE+E6v9i7?=
 =?us-ascii?Q?dUzzhwR39/0dH7ZaLNczotF3jdCaO6cEPZJq2XYirUMlUEKtSRvfU4RE+Qdw?=
 =?us-ascii?Q?SKKRS9jYJyALvLH850mxkBFj/uGdqAWexHoAl4C6pRjs7axfmxj5hry7zNiz?=
 =?us-ascii?Q?UpW3xGA3A5sRWB92BVG6egfq0W29+JAv0Kc2Mqw3NjQPTK9SZEzXkD8rZcLy?=
 =?us-ascii?Q?ZNIt8C671ioO/PHbDcJGQhBrpYSZcjWqwLu3vC7xT7Bxwgx3QuH08WYYN/ML?=
 =?us-ascii?Q?fkxAxRYZtywtJyyCoc/d6s9Cxv53LKcMqqAmPAU7/C1cw5zt/bcQauZOONi+?=
 =?us-ascii?Q?1n46R4oh2c1/9hw7LB2/mS0l+zVEM8Q0wd2sXsFLR2LVFtuN6l18hAk9gN++?=
 =?us-ascii?Q?WFQ8BzLnd9nReLW7aylATrKOrVxEXwFXMVx/HrZnhbhqRmPIwqjR+HZ3L27I?=
 =?us-ascii?Q?oDN1FBUe9/vbUNbQKazTry8DJC2UN0i2cRh4lqV3zCWJhmwCvN0lHMQik8PT?=
 =?us-ascii?Q?bH6C2kXxJVx1BdhIdu3m70HEmM4F18KpZYcjjAyjF6Y4EuQMsOyFKwrz1prP?=
 =?us-ascii?Q?fA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d68ab9ee-7928-4f05-c991-08db556768f2
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5471.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2023 17:11:23.6025
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: znda0rPhVzB29cZCk1dPOLbprCEoQFa1xGEbsY39LEzIxrJYJvC/QLcizp6egv5dV29kG+hnFWzD9Xfrw1d3zLu46bu53zNLAtBdqmpNGbw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB8422
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, May 15, 2023 at 06:17:02PM +0200, Jesper Dangaard Brouer wrote:
> 
> 
> On 12/05/2023 17.26, Larysa Zaremba wrote:
> > From: Aleksander Lobakin <aleksander.lobakin@intel.com>
> > 
> > When using XDP hints, metadata sometimes has to be much bigger
> > than 32 bytes. Relax the restriction, allow metadata larger than 32 bytes
> > and make __skb_metadata_differs() work with bigger lengths.
> > 
> > Now size of metadata is only limited by the fact it is stored as u8
> > in skb_shared_info, so maximum possible value is 255.
> 
> I'm confused, IIRC the metadata area isn't stored "in skb_shared_info".
> The maximum possible size is limited by the XDP headroom, which is also
> shared/limited with/by xdp_frame.  I must be reading the sentence wrong,
> somehow.

It's not 'metadata is stored as u8', it's 'metadata size is stored as u8' :)
Maybe I should rephrase it better in v2.

> 
> > Other important
> > conditions, such as having enough space for xdp_frame building, are already
> > checked in bpf_xdp_adjust_meta().
> > 
> > The requirement of having its length aligned to 4 bytes is still
> > valid.
> > 
> > Signed-off-by: Aleksander Lobakin <aleksander.lobakin@intel.com>
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > ---
> >   include/linux/skbuff.h | 13 ++++++++-----
> >   include/net/xdp.h      |  7 ++++++-
> >   2 files changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 8ddb4af1a501..afcd372aecdf 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -4219,10 +4219,13 @@ static inline bool __skb_metadata_differs(const struct sk_buff *skb_a,
> >   {
> >   	const void *a = skb_metadata_end(skb_a);
> >   	const void *b = skb_metadata_end(skb_b);
> > -	/* Using more efficient varaiant than plain call to memcmp(). */
> > -#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
> >   	u64 diffs = 0;
> > +	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) ||
> > +	    BITS_PER_LONG != 64)
> > +		goto slow;
> > +
> > +	/* Using more efficient variant than plain call to memcmp(). */
> >   	switch (meta_len) {
> >   #define __it(x, op) (x -= sizeof(u##op))
> >   #define __it_diff(a, b, op) (*(u##op *)__it(a, op)) ^ (*(u##op *)__it(b, op))
> > @@ -4242,11 +4245,11 @@ static inline bool __skb_metadata_differs(const struct sk_buff *skb_a,
> >   		fallthrough;
> >   	case  4: diffs |= __it_diff(a, b, 32);
> >   		break;
> > +	default:
> > +slow:
> > +		return memcmp(a - meta_len, b - meta_len, meta_len);
> >   	}
> >   	return diffs;
> > -#else
> > -	return memcmp(a - meta_len, b - meta_len, meta_len);
> > -#endif
> >   }
> >   static inline bool skb_metadata_differs(const struct sk_buff *skb_a,
> > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > index 0fbd25616241..f48723250c7c 100644
> > --- a/include/net/xdp.h
> > +++ b/include/net/xdp.h
> > @@ -370,7 +370,12 @@ xdp_data_meta_unsupported(const struct xdp_buff *xdp)
> >   static inline bool xdp_metalen_invalid(unsigned long metalen)
> >   {
> > -	return (metalen & (sizeof(__u32) - 1)) || (metalen > 32);
> > +	typeof(metalen) meta_max;
> > +
> > +	meta_max = type_max(typeof_member(struct skb_shared_info, meta_len));
> > +	BUILD_BUG_ON(!__builtin_constant_p(meta_max));
> > +
> > +	return !IS_ALIGNED(metalen, sizeof(u32)) || metalen > meta_max;
> >   }
> >   struct xdp_attachment_info {
> 
>