netdev - Re: [RFC net-next v2 4/5] net:sched:act_sample: add action cookie to sample

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] [day] [month] [year] [list]

Message-ID: <8588456f-29ac-49ae-9507-a5523a080b09@redhat.com>
Date: Thu, 11 Apr 2024 10:40:38 +0200
From: Adrian Moreno <amorenoz@...hat.com>
To: Ilya Maximets <i.maximets@....org>, netdev@...r.kernel.org
Cc: jiri@...nulli.us, xiyou.wangcong@...il.com, cmi@...dia.com,
 yotam.gi@...il.com, aconole@...hat.com, echaudro@...hat.com, horms@...nel.org
Subject: Re: [RFC net-next v2 4/5] net:sched:act_sample: add action cookie to
 sample



On 4/8/24 15:20, Ilya Maximets wrote:
> [copying my previous reply since this version actually has netdev@ in Cc]
> 
> On 4/8/24 14:57, Adrian Moreno wrote:
>> If the action has a user_cookie, pass it along to the sample so it can
>> be easily identified.
>>
>> Signed-off-by: Adrian Moreno <amorenoz@...hat.com>
>> ---
>>   net/sched/act_sample.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
> 
> Nit: some spaces in the subject would be nice.
> 
>>
>> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
>> index a69b53d54039..5c3f86ec964a 100644
>> --- a/net/sched/act_sample.c
>> +++ b/net/sched/act_sample.c
>> @@ -165,9 +165,11 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
>>   				     const struct tc_action *a,
>>   				     struct tcf_result *res)
>>   {
>> +	u8 cookie_data[TC_COOKIE_MAX_SIZE] = {};
>>   	struct tcf_sample *s = to_sample(a);
>>   	struct psample_group *psample_group;
>>   	struct psample_metadata md = {};
>> +	struct tc_cookie *user_cookie;
>>   	int retval;
>>   
>>   	tcf_lastuse_update(&s->tcf_tm);
>> @@ -189,6 +191,16 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
>>   		if (skb_at_tc_ingress(skb) && tcf_sample_dev_ok_push(skb->dev))
>>   			skb_push(skb, skb->mac_len);
>>   
>> +		rcu_read_lock();
>> +		user_cookie = rcu_dereference(a->user_cookie);
>> +		if (user_cookie) {
>> +			memcpy(cookie_data, user_cookie->data,
>> +			       user_cookie->len);
>> +			md.user_cookie = cookie_data;
>> +			md.user_cookie_len = user_cookie->len;
>> +		}
>> +		rcu_read_unlock();
> 
> Not sure what is better - extending rcu critical section
> beyond psample_sample_packet() or copying the cookie...
> What do you think?

Ha! I also scratched my head on this one.

I tried to follow all the possible paths and, as far as I could tell, there is 
no explicit sleeping involved. However, there is a call to yield() towards the 
end of netlink_broadcast_filtered() that made me think it's safer to copy the 
cookie which, in this particular case, is fairly limited in size.

Have I missed something?

> 
>> +
>>   		md.trunc_size = s->truncate ? s->trunc_size : skb->len;
>>   		psample_sample_packet(psample_group, skb, s->rate, &md);
>>   
>