netdev - Re: [PATCH net-next V1] net: track pfmemalloc drops via SKB_DROP_REASON

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: <665a622b-cb6b-4647-a287-b1b03d78e02f@kernel.org>
Date: Fri, 9 May 2025 15:48:04 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org,
 Eric Dumazet <eric.dumazet@...il.com>, "David S. Miller"
 <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>,
 Toke Høiland-Jørgensen <toke@...e.dk>,
 kernel-team@...udflare.com, mfleming@...udflare.com
Subject: Re: [PATCH net-next V1] net: track pfmemalloc drops via
 SKB_DROP_REASON_PFMEMALLOC

On 03/05/2025 01.17, Jakub Kicinski wrote:
> On Fri, 02 May 2025 17:16:38 +0200 Jesper Dangaard Brouer wrote:
>> @@ -142,15 +144,20 @@ int sk_filter_trim_cap(struct sock *sk, struct sk_buff *skb, unsigned int cap)
>>   	 */
>>   	if (skb_pfmemalloc(skb) && !sock_flag(sk, SOCK_MEMALLOC)) {
>>   		NET_INC_STATS(sock_net(sk), LINUX_MIB_PFMEMALLOCDROP);
>> +		*reason = SKB_DROP_REASON_PFMEMALLOC;
>>   		return -ENOMEM;
>>   	}
>>   	err = BPF_CGROUP_RUN_PROG_INET_INGRESS(sk, skb);
>> -	if (err)
>> +	if (err) {
>> +		*reason = SKB_DROP_REASON_SOCKET_FILTER;
>>   		return err;
>> +	}
>>   
>>   	err = security_sock_rcv_skb(sk, skb);
>> -	if (err)
>> +	if (err) {
>> +		*reason = SKB_DROP_REASON_SECURITY_HOOK;
>>   		return err;
>> +	}
>>   
>>   	rcu_read_lock();
>>   	filter = rcu_dereference(sk->sk_filter);
> 
> there is:
> 
> 	err = pkt_len ? pskb_trim(skb, max(cap, pkt_len)) : -EPERM;
> 
> later on, which I think may return error without touching drop reason.
> If caller didn't init it the reason will be undefined.

Good review, thanks for catching this.

In this (-EPERM) case should clearly set SKB_DROP_REASON_SOCKET_FILTER.
It is the original "main" reason for socket filter drop decision.
Thanks for catching my mistake.

The BPF-prog return value for type BPF_PROG_TYPE_SOCKET_FILTER is
different than more modern BPF-prog types (that usually returns an
action).  This confused me a bit when reading the code, so let link the
documentation here[1] to help reviewers and seed Google + AIs.

It states: "The return value from indicates how many bytes of the
message should be kept. Returning a value less then the side of the
packet will truncate it and returning 0 will discard the packet."

Will send a V2.

--Jesper

[1] https://docs.ebpf.io/linux/program-type/BPF_PROG_TYPE_SOCKET_FILTER/