netdev - Re: [PATCH net-next v2 2/6] net: sched: Allow extending set of supported RED flags

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87k13qxwgv.fsf@mellanox.com>
Date:   Thu, 12 Mar 2020 01:12:16 +0100
From:   Petr Machata <petrm@...lanox.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, Roman Mashak <mrv@...atatu.com>,
        jhs@...atatu.com, xiyou.wangcong@...il.com, davem@...emloft.net,
        jiri@...lanox.com, mlxsw@...lanox.com
Subject: Re: [PATCH net-next v2 2/6] net: sched: Allow extending set of supported RED flags


Jakub Kicinski <kuba@...nel.org> writes:

> On Wed, 11 Mar 2020 19:33:52 +0200 Petr Machata wrote:
>> diff --git a/include/net/red.h b/include/net/red.h
>> index 9665582c4687..5718d2b25637 100644
>> --- a/include/net/red.h
>> +++ b/include/net/red.h
>> @@ -179,6 +179,31 @@ static inline bool red_check_params(u32 qth_min, u32 qth_max, u8 Wlog)
>>  	return true;
>>  }
>>  
>> +static inline bool red_get_flags(unsigned char flags,
>> +				 unsigned char historic_mask,
>> +				 struct nlattr *flags_attr,
>> +				 unsigned int supported_mask,
>> +				 unsigned int *p_flags, unsigned char *p_userbits,
>> +				 struct netlink_ext_ack *extack)
>> +{
>> +	if (flags && flags_attr) {
>> +		NL_SET_ERR_MSG_MOD(extack, "flags should be passed either through qopt, or through a dedicated attribute");
>> +		return false;
>> +	}
>> +
>> +	*p_flags = flags & historic_mask;
>> +	if (flags_attr)
>> +		*p_flags |= nla_get_u32(flags_attr);
>
> It's less error prone for callers not to modify the output parameters
> until we're sure the call won't fail.

Ack.

>> +	if (*p_flags & ~supported_mask) {
>> +		NL_SET_ERR_MSG_MOD(extack, "unsupported RED flags specified");
>> +		return false;
>> +	}
>> +
>> +	*p_userbits = flags & ~historic_mask;
>> +	return true;
>> +}
>> +
>
>> +#define TC_RED_HISTORIC_FLAGS (TC_RED_ECN | TC_RED_HARDDROP | TC_RED_ADAPTATIVE)
>> +
>>  struct tc_red_xstats {
>>  	__u32           early;          /* Early drops */
>>  	__u32           pdrop;          /* Drops due to queue limits */
>> diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
>> index 1695421333e3..61d7c5a61279 100644
>> --- a/net/sched/sch_red.c
>> +++ b/net/sched/sch_red.c
>> @@ -35,7 +35,11 @@
>>  
>>  struct red_sched_data {
>>  	u32			limit;		/* HARD maximal queue length */
>> -	unsigned char		flags;
>> +
>> +	u32			flags;
>
> Can we stick to uchar until the number of flags grows?

No problem, but the attribute is u32.

>> +	/* Non-flags in tc_red_qopt.flags. */
>> +	unsigned char		userbits;
>> +
>>  	struct timer_list	adapt_timer;
>>  	struct Qdisc		*sch;
>>  	struct red_parms	parms;
>> @@ -44,6 +48,8 @@ struct red_sched_data {
>>  	struct Qdisc		*qdisc;
>>  };
>>  
>> +#define RED_SUPPORTED_FLAGS TC_RED_HISTORIC_FLAGS
>> +
>>  static inline int red_use_ecn(struct red_sched_data *q)
>>  {
>>  	return q->flags & TC_RED_ECN;
>> @@ -186,6 +192,7 @@ static const struct nla_policy red_policy[TCA_RED_MAX + 1] = {
>>  	[TCA_RED_PARMS]	= { .len = sizeof(struct tc_red_qopt) },
>>  	[TCA_RED_STAB]	= { .len = RED_STAB_SIZE },
>>  	[TCA_RED_MAX_P] = { .type = NLA_U32 },
>> +	[TCA_RED_FLAGS] = { .type = NLA_U32 },
>
> BITFIELD32? And then perhaps turn the define into a const validation
> data?

Nice.

> Also policy needs a .strict_start_type now.

OK.

>>  };
>>  
>>  static int red_change(struct Qdisc *sch, struct nlattr *opt,
>
>> @@ -302,7 +317,8 @@ static int red_dump(struct Qdisc *sch, struct sk_buff *skb)
>>  	struct nlattr *opts = NULL;
>>  	struct tc_red_qopt opt = {
>>  		.limit		= q->limit,
>> -		.flags		= q->flags,
>> +		.flags		= ((q->flags & TC_RED_HISTORIC_FLAGS) |
>> +				   q->userbits),
>
> nit: parens unnecessary

I'll drop them.

>>  		.qth_min	= q->parms.qth_min >> q->parms.Wlog,
>>  		.qth_max	= q->parms.qth_max >> q->parms.Wlog,
>>  		.Wlog		= q->parms.Wlog,
>> @@ -321,6 +337,8 @@ static int red_dump(struct Qdisc *sch, struct sk_buff *skb)
>>  	if (nla_put(skb, TCA_RED_PARMS, sizeof(opt), &opt) ||
>>  	    nla_put_u32(skb, TCA_RED_MAX_P, q->parms.max_P))
>>  		goto nla_put_failure;
>> +	if (q->flags & ~TC_RED_HISTORIC_FLAGS)
>> +		nla_put_u32(skb, TCA_RED_FLAGS, q->flags);
>
> Not 100% sure if conditional is needed, but please check the return
> code.

I didn't want to bother old-style clients with the new attribute.

I'll add the check.

>>  	return nla_nest_end(skb, opts);
>>  
>>  nla_put_failure: