On Thu, Oct 03, 2019 at 02:58:01PM -0700, Tom Herbert wrote:
> From: Tom Herbert <t...@quantonium.net>
>
> Create a single TLV parameter table that holds meta information for IPv6
> Hop-by-Hop and Destination TLVs. The data structure is composed of a 256
> element array of u8's (one entry for each TLV type to allow O(1)
> lookup). Each entry provides an offset into an array of TLV proc data
> structures which follows the array of u8s. The TLV proc data structure
> contains parameters and handler functions for receiving and transmitting
> TLVs. The zeroth element in the TLV proc array provides default
> parameters for TLVs.
>
> A class attribute indicates the type of extension header in which the
> TLV may be used (e.g. Hop-by-Hop options, Destination options, or
> Destination options before the routing header).
>
> Functions are defined to manipulate entries in the TLV parameter table.
>
> * tlv_{set|unset}_proc set a TLV proc entry (ops and parameters)
> * tlv_{set|unset}_params set parameters only
Perhaps it would be worth mentioning that these will
be used by a following patch in the series.
> Receive TLV lookup and processing is modified to be a lookup in the TLV
> parameter table. An init table containing parameters for TLVs supported
> by the kernel is used to initialize the TLV table.
>
> Signed-off-by: Tom Herbert <t...@herbertland.com>
Minor nit about types below, but overall this looks good to me.
Reviewed-by: Simon Horman <simon.hor...@netronome.com>
...
> diff --git a/include/net/ipeh.h b/include/net/ipeh.h
> index c1aa7b6..aaa2910 100644
> --- a/include/net/ipeh.h
> +++ b/include/net/ipeh.h
...
> +/* ipeh_tlv_get_proc_by_type assumes rcu_read_lock is held */
> +static inline struct tlv_proc *ipeh_tlv_get_proc_by_type(
> + struct tlv_param_table *tlv_param_table, unsigned char type)
> +{
> + struct tlv_param_table_data *tpt =
> + rcu_dereference(tlv_param_table->data);
> +
> + return &tpt->types[tpt->entries[type]].proc;
> +}
> +
> +/* ipeh_tlv_get_proc assumes rcu_read_lock is held */
> +static inline struct tlv_proc *ipeh_tlv_get_proc(
> + struct tlv_param_table *tlv_param_table,
> + const __u8 *tlv)
> +{
> + return ipeh_tlv_get_proc_by_type(tlv_param_table, tlv[0]);
The type of tlv is const __u8 *, however,
1. the type of the 'type' parameter of ipeh_tlv_get_proc_by_type is
unsigned char; and
2. this function is passed a value of type const unsigned char * by
ipeh_parse_tlv().
I understand these are all one byte unsigned integer types.
But perhaps this could be made more consistent somehow.
> +}
...
> diff --git a/net/ipv6/exthdrs_common.c b/net/ipv6/exthdrs_common.c
> index 99a0911..43737fc 100644
> --- a/net/ipv6/exthdrs_common.c
> +++ b/net/ipv6/exthdrs_common.c
..
> @@ -221,26 +226,22 @@ bool ipeh_parse_tlv(const struct tlvtype_proc *procs,
> struct sk_buff *skb,
>
> tlv_count++;
> if (tlv_count > max_count &&
> - parse_error(skb, off, IPEH_PARSE_ERR_OPT_TOOMANY))
> + !parse_error(skb, off, IPEH_PARSE_ERR_OPT_TOOMANY))
> goto bad;
>
> - for (curr = procs; curr->type >= 0; curr++) {
> - if (curr->type == nh[off]) {
> - /* type specific length/alignment
> - * checks will be performed in the
> - * func().
> - */
> - if (curr->func(skb, off) == false)
> - return false;
> - break;
> - }
> - }
> - if (curr->type < 0 &&
> - !parse_error(skb, off,
> + curr = ipeh_tlv_get_proc(tlv_param_table, &nh[off]);
> + if ((curr->params.r.class & class) && curr->ops.func) {
> + /* Handler will apply additional checks to
> + * the TLV
> + */
> + if (!curr->ops.func(class, skb, off))
> + return false;
> + } else if (!parse_error(skb, off,
> disallow_unknowns ?
> IPEH_PARSE_ERR_OPT_UNK_DISALW :
> - IPEH_PARSE_ERR_OPT_UNK))
> + IPEH_PARSE_ERR_OPT_UNK)) {
> goto bad;
> + }
>
> padlen = 0;
> break;
