Message-ID: <fa52e3654a44dd250437ebe3e8397bff95399893.camel@huaweicloud.com>
Date: Tue, 21 Jan 2025 14:48:09 +0100
From: Roberto Sassu <roberto.sassu@...weicloud.com>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: zohar@...ux.ibm.com, dmitry.kasatkin@...il.com,
 eric.snowberg@...cle.com,  corbet@....net, mcgrof@...nel.org,
 petr.pavlu@...e.com, samitolvanen@...gle.com,  da.gomez@...sung.com,
 akpm@...ux-foundation.org, paul@...l-moore.com,  jmorris@...ei.org,
 serge@...lyn.com, shuah@...nel.org, mcoquelin.stm32@...il.com, 
 alexandre.torgue@...s.st.com, linux-integrity@...r.kernel.org, 
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, 
 linux-api@...r.kernel.org, linux-modules@...r.kernel.org, 
 linux-security-module@...r.kernel.org, linux-kselftest@...r.kernel.org, 
 wufan@...ux.microsoft.com, pbrobinson@...il.com, zbyszek@...waw.pl,
 hch@....de,  mjg59@...f.ucam.org, pmatilai@...hat.com, jannh@...gle.com,
 dhowells@...hat.com,  jikos@...nel.org, mkoutny@...e.com, ppavlu@...e.com,
 petr.vorel@...il.com,  mzerqung@...inter.de, kgold@...ux.ibm.com, Roberto
 Sassu <roberto.sassu@...wei.com>
Subject: Re: [PATCH v6 01/15] lib: Add TLV parser

On Tue, 2025-01-21 at 14:29 +0100, Thomas Weißschuh wrote:
> Hi Robert,
> 
> On 2024-11-19 11:49:08+0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@...wei.com>
> > 
> > Add a parser of a generic Type-Length-Value (TLV) format:
> > 
> > +--------------+--+---------+--------+---------+
> > > field1 (u16) | len1 (u32) | value1 (u8 len1) |
> > +--------------+------------+------------------+
> > >     ...      |    ...     |        ...       |
> > +--------------+------------+------------------+
> > > fieldN (u16) | lenN (u32) | valueN (u8 lenN) |
> > +--------------+------------+------------------+
> 
> Should mention that its big endian.

Ok.

> > Each adopter can define its own fields. The TLV parser does not need to be
> > aware of those, but lets the adopter obtain the data and decide how to
> 
> "adopter" -> "user".

Ok.

> > continue.
> > 
> > After processing a TLV entry, call the callback function also with the
> > callback data provided by the adopter. The latter can decide how to
> > interpret the TLV entry depending on the field ID.
> > 
> > Nesting TLVs is also possible, the callback function can call tlv_parse()
> > to parse the inner structure.
> 
> Given that we already have the netlink data structures, helpers and
> infrastructure, what is the advantage over those?

Sorry, I'm not too familiar on how netlink works, so I might not
understand your point.

I think the benefit of this data structure is the retrocompatibility.
If you add new data fields, you don't need to introduce a v2, v3 data
format.

New versions of the parser can consume the new information, while the
older can still take the ones they are able to understand.

Thanks

Roberto

> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
> > ---
> >  MAINTAINERS                     |  8 +++
> >  include/linux/tlv_parser.h      | 32 ++++++++++++
> >  include/uapi/linux/tlv_parser.h | 41 ++++++++++++++++
> >  lib/Kconfig                     |  3 ++
> >  lib/Makefile                    |  2 +
> >  lib/tlv_parser.c                | 87 +++++++++++++++++++++++++++++++++
> >  lib/tlv_parser.h                | 18 +++++++
> >  7 files changed, 191 insertions(+)
> >  create mode 100644 include/linux/tlv_parser.h
> >  create mode 100644 include/uapi/linux/tlv_parser.h
> >  create mode 100644 lib/tlv_parser.c
> >  create mode 100644 lib/tlv_parser.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a097afd76ded..1f7ffa1c9dbd 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -23388,6 +23388,14 @@ W:	http://sourceforge.net/projects/tlan/
> >  F:	Documentation/networking/device_drivers/ethernet/ti/tlan.rst
> >  F:	drivers/net/ethernet/ti/tlan.*
> >  
> > +TLV PARSER
> > +M:	Roberto Sassu <roberto.sassu@...wei.com>
> > +L:	linux-kernel@...r.kernel.org
> > +S:	Maintained
> > +F:	include/linux/tlv_parser.h
> > +F:	include/uapi/linux/tlv_parser.h
> > +F:	lib/tlv_parser.*
> > +
> >  TMIO/SDHI MMC DRIVER
> >  M:	Wolfram Sang <wsa+renesas@...g-engineering.com>
> >  L:	linux-mmc@...r.kernel.org
> > diff --git a/include/linux/tlv_parser.h b/include/linux/tlv_parser.h
> > new file mode 100644
> > index 000000000000..0c72742af548
> > --- /dev/null
> > +++ b/include/linux/tlv_parser.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@...wei.com>
> > + *
> > + * Header file of TLV parser.
> > + */
> > +
> > +#ifndef _LINUX_TLV_PARSER_H
> > +#define _LINUX_TLV_PARSER_H
> > +
> > +#include <uapi/linux/tlv_parser.h>
> > +
> > +/**
> > + * typedef callback - Callback after parsing TLV entry
> > + * @callback_data: Opaque data to supply to the callback function
> > + * @field: Field identifier
> > + * @field_data: Field data
> > + * @field_len: Length of @field_data
> > + *
> > + * This callback is invoked after a TLV entry is parsed.
> > + *
> > + * Return: Zero on success, a negative value on error.
> 
> It's not explained what happens on error.
> 
> > + */
> > +typedef int (*callback)(void *callback_data, __u16 field,
> > +			const __u8 *field_data, __u32 field_len);
> 
> No need for __underscore types in kernel-only signatures.
> 
> > +
> > +int tlv_parse(callback callback, void *callback_data, const __u8 *data,
> > +	      size_t data_len, const char **fields, __u32 num_fields);
> > +
> > +#endif /* _LINUX_TLV_PARSER_H */
> > diff --git a/include/uapi/linux/tlv_parser.h b/include/uapi/linux/tlv_parser.h
> > new file mode 100644
> > index 000000000000..171d0cfd2c4c
> > --- /dev/null
> > +++ b/include/uapi/linux/tlv_parser.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@...wei.com>
> > + *
> > + * Implement the user space interface for the TLV parser.
> > + */
> 
> Can you explain in the commit message where this will be exposed to
> userspace as binary?
> 
> > +
> > +#ifndef _UAPI_LINUX_TLV_PARSER_H
> > +#define _UAPI_LINUX_TLV_PARSER_H
> > +
> > +#include <linux/types.h>
> > +
> > +/*
> > + * TLV format:
> > + *
> > + * +--------------+--+---------+--------+---------+
> > + * | field1 (u16) | len1 (u32) | value1 (u8 len1) |
> > + * +--------------+------------+------------------+
> > + * |     ...      |    ...     |        ...       |
> > + * +--------------+------------+------------------+
> > + * | fieldN (u16) | lenN (u32) | valueN (u8 lenN) |
> > + * +--------------+------------+------------------+
> > + */
> > +
> > +/**
> > + * struct tlv_entry - Entry of TLV format
> > + * @field: Field identifier
> > + * @length: Data length
> > + * @data: Data
> > + *
> > + * This structure represents an entry of the TLV format.
> > + */
> > +struct tlv_entry {
> > +	__u16 field;
> > +	__u32 length;
> 
> Use __be16 and __be32 here.
> 
> > +	__u8 data[];
> 
> __counted_by()?
> Not sure how this interacts with __be.
> 
> > +} __attribute__((packed));
> > +
> > +#endif /* _UAPI_LINUX_TLV_PARSER_H */
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index b38849af6f13..9141dcfc1704 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -777,3 +777,6 @@ config POLYNOMIAL
> >  
> >  config FIRMWARE_TABLE
> >  	bool
> > +
> > +config TLV_PARSER
> > +	bool
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 773adf88af41..630de494eab5 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -393,5 +393,7 @@ obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o
> >  obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
> >  
> >  obj-$(CONFIG_FIRMWARE_TABLE) += fw_table.o
> > +obj-$(CONFIG_TLV_PARSER) += tlv_parser.o
> > +CFLAGS_tlv_parser.o += -I lib
> 
> Does this work with out of tree builds?
> 
> >  
> >  subdir-$(CONFIG_FORTIFY_SOURCE) += test_fortify
> > diff --git a/lib/tlv_parser.c b/lib/tlv_parser.c
> > new file mode 100644
> > index 000000000000..dbbe08018b4d
> > --- /dev/null
> > +++ b/lib/tlv_parser.c
> > @@ -0,0 +1,87 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@...wei.com>
> > + *
> > + * Implement the TLV parser.
> > + */
> > +
> > +#define pr_fmt(fmt) "tlv_parser: "fmt
> > +#include <tlv_parser.h>
> 
> This should be "tlv_parser.h",
> but the header files looks unnecessary in the first place.
> 
> > +
> > +/**
> > + * tlv_parse - Parse TLV data
> > + * @callback: Callback function to call to parse the entries
> > + * @callback_data: Opaque data to supply to the callback function
> > + * @data: Data to parse
> > + * @data_len: Length of @data
> > + * @fields: Array of field strings
> > + * @num_fields: Number of elements of @fields
> > + *
> > + * Parse the TLV data format and call the supplied callback function for each
> > + * entry, passing also the opaque data pointer.
> > + *
> > + * The callback function decides how to process data depending on the field.
> 
> Mention that a callback return an error will abort the whole parsing.
> 
> > + *
> > + * Return: Zero on success, a negative value on error.
> > + */
> > +int tlv_parse(callback callback, void *callback_data, const __u8 *data,
> > +	      size_t data_len, const char **fields, __u32 num_fields)
> 
> No need for __underscore types in kernel-only functions.
> 
> "num_fields" and "fields" are accessed without checking for validity.
> 
> "fields" is only every used for debug logging, so can be removed.
> "num_fields" probably, too.
> 
> > +{
> > +	const __u8 *data_ptr = data;
> > +	struct tlv_entry *entry;
> 
> This comes from the input data, should also be const.
> 
> > +	__u16 parsed_field;
> > +	__u32 len;
> 
> field_len
> 
> > +	int ret;
> > +
> > +	if (data_len > U32_MAX) {
> > +		pr_debug("Data too big, size: %zd\n", data_len);
> > +		return -E2BIG;
> > +	}
> > +
> > +	while (data_len) {
> > +		if (data_len < sizeof(*entry))
> > +			return -EBADMSG;
> > +
> > +		entry = (struct tlv_entry *)data_ptr;
> > +		data_ptr += sizeof(*entry);
> > +		data_len -= sizeof(*entry);
> > +
> > +		parsed_field = __be16_to_cpu(entry->field);
> 
> This doesn't seem to handle invalid alignment, some architectures will
> trap unaligned accesses.
> Depending on the size and usage patterns it may make sense to document
> some alignment recommendations/requirements.
> (Not sure how big of a performance difference it would make)
> 
> > +		if (parsed_field >= num_fields) {
> > +			pr_debug("Invalid field %u, max: %u\n",
> > +				 parsed_field, num_fields - 1);
> > +			return -EBADMSG;
> > +		}
> > +
> > +		len = __be32_to_cpu(entry->length);
> > +
> > +		if (data_len < len)
> > +			return -EBADMSG;
> > +
> > +		pr_debug("Data: field: %s, len: %u\n", fields[parsed_field],
> > +			 len);
> > +
> > +		if (!len)
> > +			continue;
> 
> Empty fields are discarded silently, is this intentional?
> It should be documented. Those fields could be useful for flag data.
> 
> > +
> > +		ret = callback(callback_data, parsed_field, data_ptr, len);
> > +		if (ret < 0) {
> > +			pr_debug("Parsing of field %s failed, ret: %d\n",
> > +				 fields[parsed_field], ret);
> > +			return ret;
> > +		}
> > +
> > +		data_ptr += len;
> > +		data_len -= len;
> > +	}
> > +
> > +	if (data_len) {
> 
> Can this ever happen?
> The check at the beginning of the loop should have caught it already.
> 
> > +		pr_debug("Excess data: %zu bytes\n", data_len);
> > +		return -EBADMSG;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(tlv_parse);
> 
> Some kunit tests would be great.
> 
> > diff --git a/lib/tlv_parser.h b/lib/tlv_parser.h
> > new file mode 100644
> > index 000000000000..e663966deac5
> > --- /dev/null
> > +++ b/lib/tlv_parser.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@...wei.com>
> > + *
> > + * Header file of TLV parser.
> > + */
> > +
> > +#ifndef _LIB_TLV_PARSER_H
> > +#define _LIB_TLV_PARSER_H
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/err.h>
> > +#include <linux/limits.h>
> > +#include <linux/tlv_parser.h>
> 
> The #includes should move to the .c file and the header be removed.
> 
> > +
> > +#endif /* _LIB_TLV_PARSER_H */
> > -- 
> > 2.47.0.118.gfd3785337b
> > 

Powered by blists - more mailing lists