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

[Jesper Dangaard Brouer <hawk@kernel.org>]

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/