netdev - Re: [PATCH net-next v15 12/15] net: homa: create homa

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: <CAGXJAmzUFH8RcgGf6tz=BCh_VBQXTg4o7M3HSthdRjD7_eHKoQ@mail.gmail.com>
Date: Fri, 5 Sep 2025 14:32:18 -0700
From: John Ousterhout <ouster@...stanford.edu>
To: Eric Dumazet <edumazet@...gle.com>
Cc: netdev@...r.kernel.org, pabeni@...hat.com, horms@...nel.org, 
	kuba@...nel.org
Subject: Re: [PATCH net-next v15 12/15] net: homa: create homa_incoming.c

On Tue, Sep 2, 2025 at 12:19 AM Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > +/**
> > + * homa_message_in_init() - Constructor for homa_message_in.
> > + * @rpc:          RPC whose msgin structure should be initialized. The
> > + *                msgin struct is assumed to be zeroes.
> > + * @length:       Total number of bytes in message.
> > + * Return:        Zero for successful initialization, or a negative errno
> > + *                if rpc->msgin could not be initialized.
> > + */
> > +int homa_message_in_init(struct homa_rpc *rpc, int length)
> > +       __must_hold(rpc->bucket->lock)
> > +{
> > +       int err;
> > +
> > +       if (length > HOMA_MAX_MESSAGE_LENGTH)
> > +               return -EINVAL;
> > +
> > +       rpc->msgin.length = length;
> > +       skb_queue_head_init(&rpc->msgin.packets);
>
> Do you need the lock, or can you use __skb_queue_head_init() here for clarity ?

No need for the lock. I hadn't realized that I should then use a
different initializer. I have now switched to __skb_queue_head_init
(and added to the documentation for packets).

> > +               if (n == 0) {
> > +                       atomic_andnot(RPC_PKTS_READY, &rpc->flags);
>
> All networking uses clear_bit() instead...

I have switched everywhere.

> > +               n = 0;
>
> > +               atomic_or(APP_NEEDS_LOCK, &rpc->flags);
> > +               homa_rpc_lock(rpc);
> > +               atomic_andnot(APP_NEEDS_LOCK, &rpc->flags);
>
> This construct would probably need a helper.

Done, here and elsewhere.

> > +
> > +done:
> > +       kfree_skb(skb);
>
>
> Please double check all your kfree_skb() vs consume_skb()
>
> perf record -a -e skb:kfree_skb  sleep 60
> vs
> perf record -a -e skb:consume_skb  sleep 60
>
> As a bonus, you can use kfree_skb_reason(skb, some_reason) for future
> bug hunting

I wasn't aware of the consume_skb/kfree_skb_reason pattern. I've
checked all uses of kfree_skb and converted to consume_skb where
appropriate. The code also uses kfree_skb_reason wherever there is an
appropriate reason code.

-John-