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: <20250116040932.96265-1-kuniyu@amazon.com>
Date: Thu, 16 Jan 2025 13:09:32 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <pabeni@...hat.com>
CC: <davem@...emloft.net>, <donald.hunter@...hat.com>, <edumazet@...gle.com>,
	<horms@...nel.org>, <kuba@...nel.org>, <kuni1840@...il.com>,
	<kuniyu@...zon.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 net-next 06/11] af_unix: Set drop reason in unix_stream_sendmsg().

From: Paolo Abeni <pabeni@...hat.com>
Date: Wed, 15 Jan 2025 16:12:59 +0100
> 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.

Thanks for the feedback, all!

I'll change kfree_skb() to consume_skb() in

  * unix_stream_connect()
  * unix_stream_sendmsg()
  * queue_oob()
  * unix_dgram_sendmsg()

where we can distinguish what happened from errno.