Re: [PATCH bpf-next 06/11] bpfilter: Add struct match

[Dmitrii Banshchikov <me@ubique.spb.ru>]

On Thu, May 20, 2021 at 04:26:28AM +0000, Song Liu wrote:
> 
> 
> > On May 17, 2021, at 3:53 PM, Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
> > 
> > struct match_ops defines polymorphic interface for matches. A match
> > consists of pointers to struct match_ops and struct xt_entry_match which
> > contains a payload for the match's type.
> > 
> > All match_ops are kept in map match_ops_map by their name.
> > 
> > Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> > 
> [...]
> 
> > diff --git a/net/bpfilter/match-ops-map.h b/net/bpfilter/match-ops-map.h
> > new file mode 100644
> > index 000000000000..0ff57f2d8da8
> > --- /dev/null
> > +++ b/net/bpfilter/match-ops-map.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2021 Telegram FZ-LLC
> > + */
> > +
> > +#ifndef NET_BPFILTER_MATCH_OPS_MAP_H
> > +#define NET_BPFILTER_MATCH_OPS_MAP_H
> > +
> > +#include "map-common.h"
> > +
> > +#include <linux/err.h>
> > +
> > +#include <errno.h>
> > +#include <string.h>
> > +
> > +#include "match.h"
> > +
> > +struct match_ops_map {
> > +	struct hsearch_data index;
> > +};
> 
> Do we plan to extend match_ops_map? Otherwise, we can just use 
> hsearch_data in struct context. 

Agreed.

> 
> > +
> > +static inline int create_match_ops_map(struct match_ops_map *map, size_t nelem)
> > +{
> > +	return create_map(&map->index, nelem);
> > +}
> > +
> > +static inline const struct match_ops *match_ops_map_find(struct match_ops_map *map,
> > +							 const char *name)
> > +{
> > +	const size_t namelen = strnlen(name, BPFILTER_EXTENSION_MAXNAMELEN);
> > +
> > +	if (namelen < BPFILTER_EXTENSION_MAXNAMELEN)
> > +		return map_find(&map->index, name);
> > +
> > +	return ERR_PTR(-EINVAL);
> > +}
> > +
> > +static inline int match_ops_map_insert(struct match_ops_map *map, const struct match_ops *match_ops)
> > +{
> > +	return map_insert(&map->index, match_ops->name, (void *)match_ops);
> > +}
> > +
> > +static inline void free_match_ops_map(struct match_ops_map *map)
> > +{
> > +	free_map(&map->index);
> > +}
> > +
> > +#endif // NET_BPFILTER_MATCT_OPS_MAP_H
> > diff --git a/net/bpfilter/match.c b/net/bpfilter/match.c
> > new file mode 100644
> > index 000000000000..aeca1b93cd2d
> > --- /dev/null
> > +++ b/net/bpfilter/match.c
> > @@ -0,0 +1,73 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2021 Telegram FZ-LLC
> > + */
> > +
> > +#define _GNU_SOURCE
> > +
> > +#include "match.h"
> > +
> > +#include <linux/err.h>
> > +#include <linux/netfilter/xt_tcpudp.h>
> 
> Besides xt_ filters, do we plan to support others? If so, we probably 
> want separate files for each of them. 

Do you mean nft filters?
They use nfilter API and currently we cannot hook into it - so
probably eventually.


> 
> > +
> > +#include <errno.h>
> > +#include <string.h>
> > +
> > +#include "bflog.h"
> > +#include "context.h"
> > +#include "match-ops-map.h"
> > +
> > +#define BPFILTER_ALIGN(__X) __ALIGN_KERNEL(__X, __alignof__(__u64))
> > +#define MATCH_SIZE(type) (sizeof(struct bpfilter_ipt_match) + BPFILTER_ALIGN(sizeof(type)))
> > +
> > +static int udp_match_check(struct context *ctx, const struct bpfilter_ipt_match *ipt_match)
> > +{
> > +	const struct xt_udp *udp;
> > +
> > +	udp = (const struct xt_udp *)&ipt_match->data;
> > +
> > +	if (udp->invflags & XT_UDP_INV_MASK) {
> > +		BFLOG_DEBUG(ctx, "cannot check match 'udp': invalid flags\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +const struct match_ops udp_match_ops = { .name = "udp",
> 
> And maybe we should name this one "xt_udp"? 

Agreed.


> 
> > +					 .size = MATCH_SIZE(struct xt_udp),
> > +					 .revision = 0,
> > +					 .check = udp_match_check };
> > +
> > +int init_match(struct context *ctx, const struct bpfilter_ipt_match *ipt_match, struct match *match)
> > +{
> > +	const size_t maxlen = sizeof(ipt_match->u.user.name);
> > +	const struct match_ops *found;
> > +	int err;
> > +
> > +	if (strnlen(ipt_match->u.user.name, maxlen) == maxlen) {
> > +		BFLOG_DEBUG(ctx, "cannot init match: too long match name\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	found = match_ops_map_find(&ctx->match_ops_map, ipt_match->u.user.name);
> > +	if (IS_ERR(found)) {
> > +		BFLOG_DEBUG(ctx, "cannot find match by name: '%s'\n", ipt_match->u.user.name);
> > +		return PTR_ERR(found);
> > +	}
> > +
> > +	if (found->size != ipt_match->u.match_size ||
> > +	    found->revision != ipt_match->u.user.revision) {
> > +		BFLOG_DEBUG(ctx, "invalid match: '%s'\n", ipt_match->u.user.name);
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = found->check(ctx, ipt_match);
> > +	if (err)
> > +		return err;
> > +
> > +	match->match_ops = found;
> > +	match->ipt_match = ipt_match;
> > +
> > +	return 0;
> > +}
> > diff --git a/net/bpfilter/match.h b/net/bpfilter/match.h
> > new file mode 100644
> > index 000000000000..79b7c87016d4
> > --- /dev/null
> > +++ b/net/bpfilter/match.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2021 Telegram FZ-LLC
> > + */
> > +
> > +#ifndef NET_BPFILTER_MATCH_H
> > +#define NET_BPFILTER_MATCH_H
> > +
> > +#include "../../include/uapi/linux/bpfilter.h"
> > +
> > +#include <stdint.h>
> > +
> > +struct bpfilter_ipt_match;
> > +struct context;
> > +struct match_ops_map;
> > +
> > +struct match_ops {
> > +	char name[BPFILTER_EXTENSION_MAXNAMELEN];
> 
> BPFILTER_EXTENSION_MAXNAMELEN is 29, so "size" below is mis-aligned. I guess
> we can swap size and revision. 

Agreed.

> 
> > +	uint16_t size;
> > +	uint8_t revision;
> > +	int (*check)(struct context *ctx, const struct bpfilter_ipt_match *ipt_match);
> > +};
> > +
> > +extern const struct match_ops udp_match_ops;
> > +
> > +struct match {
> > +	const struct match_ops *match_ops;
> > +	const struct bpfilter_ipt_match *ipt_match;
> > +};
> 
> [...]
> 

-- 

Dmitrii Banshchikov