Message-ID: <CAGXJAmxqoPw8iTH0Bt4z5V2feM8rekDDOJapek4eyMuLJhUAtA@mail.gmail.com>
Date: Mon, 27 Jan 2025 16:40:47 -0800
From: John Ousterhout <ouster@...stanford.edu>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, edumazet@...gle.com, horms@...nel.org, 
	kuba@...nel.org
Subject: Re: [PATCH net-next v6 07/12] net: homa: create homa_sock.h and homa_sock.c

On Thu, Jan 23, 2025 at 11:02 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > +struct homa_sock *homa_socktab_next(struct homa_socktab_scan *scan)
> > +{
> > +     struct homa_socktab_links *links;
> > +     struct homa_sock *hsk;
> > +
> > +     while (1) {
> > +             while (!scan->next) {
> > +                     struct hlist_head *bucket;
> > +
> > +                     scan->current_bucket++;
> > +                     if (scan->current_bucket >= HOMA_SOCKTAB_BUCKETS)
> > +                             return NULL;
> > +                     bucket = &scan->socktab->buckets[scan->current_bucket];
> > +                     scan->next = (struct homa_socktab_links *)
> > +                                   rcu_dereference(hlist_first_rcu(bucket));
>
> The only caller for this function so far is not under RCU lock: you
> should see a splat here if you build and run this code with:
>
> CONFIG_LOCKDEP=y
>
> (which in turn is highly encouraged)

Strange... I have had CONFIG_LOCKDEP enabled for a while now, but for
some reason I didn't see a flag for that. In any case, all of the
callers to homa_socktab_next now hold the RCU lock (I fixed this
during my scan of RCU usage in response to one of your earlier
messages for this patch series).

> > +             }
> > +             links = scan->next;
> > +             hsk = links->sock;
> > +             scan->next = (struct homa_socktab_links *)
> > +                             rcu_dereference(hlist_next_rcu(&links->hash_links));
>
> homa_socktab_links is embedded into the home sock; if the RCU protection
> is released and re-acquired after a homa_socktab_next() call, there is
> no guarantee links/hsk are still around and the above statement could
> cause UaF.

There is code in homa_sock_unlink to deal with this: it makes a pass
over all of the active scans, and if the "next" field in any
homa_socktab_scan refers to the socket being deleted, it updates the
"next" field to refer to the next socket after the one being deleted.
Thus the "next" fields are always valid, even in the face of socket
deletion.

> This homa_socktab things looks quite complex. A simpler implementation
> could use a simple RCU list _and_ acquire a reference to the hsk before
> releasing the RCU lock.

I agree that this is complicated. But I can't see a simpler solution.
The problem is that we need to iterate through all of the sockets and
release the RCU lock at some points during the iteration. The problem
isn't preserving the current hsk; it's preserving the validity of the
pointer to the next one also. I don't fully understand what you're
proposing above; if you can make it a bit more precise I'll see if it
solves all the problems I'm aware of and does it in a simpler way.

> > +int homa_sock_init(struct homa_sock *hsk, struct homa *homa)
> > +{
> > +     struct homa_socktab *socktab = homa->port_map;
> > +     int starting_port;
> > +     int result = 0;
> > +     int i;
> > +
> > +     spin_lock_bh(&socktab->write_lock);
>
> A single contended lock for the whole homa sock table? Why don't you use
> per bucket locks?

Creating a socket is a very rare operation: it happens roughly once in
the lifetime of each application. Thus per bucket locks aren't
necessary. Homa is very different from TCP in this regard.

> [...]
> > +struct homa_rpc_bucket {
> > +     /**
> > +      * @lock: serves as a lock both for this bucket (e.g., when
> > +      * adding and removing RPCs) and also for all of the RPCs in
> > +      * the bucket. Must be held whenever manipulating an RPC in
> > +      * this bucket. This dual purpose permits clean and safe
> > +      * deletion and garbage collection of RPCs.
> > +      */
> > +     spinlock_t lock;
> > +
> > +     /** @rpcs: list of RPCs that hash to this bucket. */
> > +     struct hlist_head rpcs;
> > +
> > +     /**
> > +      * @id: identifier for this bucket, used in error messages etc.
> > +      * It's the index of the bucket within its hash table bucket
> > +      * array, with an additional offset to separate server and
> > +      * client RPCs.
> > +      */
> > +     int id;
>
> On 64 bit arches this struct will have 2 4-bytes holes. If you reorder
> the field:
>         spinlock_t lock;
>         int id;
>         struct hlist_head rpcs;
>
> the struct size will decrease by 8 bytes.

Done. I wasn't aware that spinlock_t is so tiny.

> > +struct homa_sock {
> > +     /* Info for other network layers. Note: IPv6 info (struct ipv6_pinfo
> > +      * comes at the very end of the struct, *after* Homa's data, if this
> > +      * socket uses IPv6).
> > +      */
> > +     union {
> > +             /** @sock: generic socket data; must be the first field. */
> > +             struct sock sock;
> > +
> > +             /**
> > +              * @inet: generic Internet socket data; must also be the
> > +              first field (contains sock as its first member).
> > +              */
> > +             struct inet_sock inet;
> > +     };
>
> Why adding this union? Just
>         struct inet_sock inet;
> would do.

It's not technically necessary, but it allows code to refer to the
struct sock as hsk->sock rather than hsk->inet.sk (saves me having to
visit struct inet to remember the field name for the struct sock). Not
a big deal, I'll admit...

-John-

Powered by blists - more mailing lists