Re: [PATCH v2 net-next 06/11] af_unix: Set drop reason in unix_stream_sendmsg().

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Paolo Abeni <pabeni@redhat.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@kernel.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.