netdev - Re: [PATCH v2 net-next 06/11] af_unix: Set drop reason in unix_stream

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: <eb30b164-7f86-46bf-a5d3-0f8bda5e9398@redhat.com>
Date: Wed, 15 Jan 2025 16:12:59 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Donald Hunter <donald.hunter@...hat.com>, Jakub Kicinski <kuba@...nel.org>
Cc: Kuniyuki Iwashima <kuniyu@...zon.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Simon Horman <horms@...nel.org>, Kuniyuki Iwashima <kuni1840@...il.com>,
 netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 06/11] af_unix: Set drop reason in
 unix_stream_sendmsg().

On 1/15/25 2:52 PM, Donald Hunter wrote:
> On Tue, 14 Jan 2025 at 20:05, Jakub Kicinski <kuba@...nel.org> wrote:
>>
>> On Sun, 12 Jan 2025 13:08:05 +0900 Kuniyuki Iwashima wrote:
>>> @@ -2249,14 +2265,13 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other
>>>  static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
>>>                              size_t len)
>>>  {
>>> +     enum skb_drop_reason reason;
>>
>> I feel like we should draw the line somewhere for the reason codes.
>> We started with annotating packet drops in the stack, which are
>> otherwise hard to notice, we don't even have counters for all of them.
>> But at this point we're annotating sendmsg() errors? The fact we free
>> an skb on the error path seems rather coincidental for a sendmsg error.
>> IOW aren't we moving from packet loss annotation into general tracing
>> territory here?
>>
>> If there is no ambiguity and application will get an error from a system
>> call I'd just use consume_skb().
>>
>> I'm probably the most resistant to the drop reason codes, so I defer
>> to Paolo / Eric for the real judgment...
> 
> For what it's worth, I agree that there's no need to annotate a drop
> reason for sendmsg failures that return error codes to the caller.
> That's why my original patch proposal just changed them to use
> consume_skb(). I did misrepresent the cases as "happy path" but I
> really meant that from the perspective of "no send initiated, so no
> drop reason".
> 
> https://lore.kernel.org/netdev/20241116094236.28786-1-donald.hunter@gmail.com/

I also agree with Jakub with a slightly different reasoning. IMHO drop
reason goal is to let user-space easily understand where/why skbs are
dropped. If the drop reason reflects a syscall error code, the
user-space already has all the info.

IIRC the general guidance agreed upon in the last Netconf was to add
drop reasons when we can't distinguish multiple kind of drops within the
same function. IMHO such guidance fits with not using drop reason in
this specific case: as said we can discriminate the errors via the
syscall error code.

Cheers,

Paolo