Re: [PATCH v4 07/12] xen/spinlock: add explicit non-recursive locking functions

[Juergen Gross <jgross@xxxxxxxx>]

On 13.12.23 09:36, Julien Grall wrote:
> On 13/12/2023 06:17, Juergen Gross wrote:
> > On 12.12.23 19:49, Julien Grall wrote:
> > > Hi Juergen,
> > >
> > > On 12/12/2023 09:47, Juergen Gross wrote:
> > > > -#define spin_lock_init_prof(s, l) __spin_lock_init_prof(s, l, spinlock_t)
> > > > -#define rspin_lock_init_prof(s, l) __spin_lock_init_prof(s, l, rspinlock_t)
> > > > +#define spin_lock_init_prof(s, 
> > > > l)                                             \
> > > > +    __spin_lock_init_prof(s, l, lock, spinlock_t, 0)
> > > > +#define rspin_lock_init_prof(s, 
> > > > l)                                            \
> > > > +    __spin_lock_init_prof(s, l, rlock, rspinlock_t, 1)
> > > >   void _lock_profile_register_struct(
> > > >       int32_t type, struct lock_profile_qhead *qhead, int32_t idx);
> > > > @@ -174,6 +179,7 @@ struct lock_profile_qhead { };
> > > >   #endif
> > > > +
> > > Spurious change?
> > Indeed, will remove it.
> >
> > > >   typedef union {
> > > >       uint32_t head_tail;
> > > >       struct {
> > > > @@ -261,4 +267,12 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, 
> > > > unsigned long flags);
> > > >   /* Ensure a lock is quiescent between two critical operations. */
> > > >   #define spin_barrier(l)               _spin_barrier(l)
> > > > +#define nrspin_trylock(l)    spin_trylock(l)
> > > > +#define nrspin_lock(l)       spin_lock(l)
> > > > +#define nrspin_unlock(l)     spin_unlock(l)
> > > > +#define nrspin_lock_irq(l)   spin_lock_irq(l)
> > > > +#define nrspin_unlock_irq(l) spin_unlock_irq(l)
> > > > +#define nrspin_lock_irqsave(l, f)      spin_lock_irqsave(l, f)
> > > > +#define nrspin_unlock_irqrestore(l, f) spin_unlock_irqrestore(l, f)
> > > There is a comment describing [r]spinlock but not this new variant. Can you 
> > > add one?
> > Okay.
> >
> > > That said, I know this is existing code, but I have to admit this is a bit 
> > > unclear why we are allowing an recursive spinlock to be non-recursive. To me 
> > > it sounds like we are making the typesafe not very safe because it doesn't 
> > > guarantee that we are not mixing the call nrspin_* with rspin_*.
> > This is the current API.
> I know. This is why I wrote "I know this is existing code".
>
> > If you have locked with nrspin_*, any rspin_* attempt on the same lock will
> > spin until rspin_unlock (nrspin_* will not set recurse_cpu, but take the
> > lock).
> >
> > If you have locked with rspin_*, any nrspin_* attempt on the same lock will
> > spin, too.
> >
> > So I don't see any major problem regarding accidental misuse, which wouldn't
> > be visible by deadlocks (there is no silent misbehavior).
> Right, so this will lead to a deadlock, which is my concern. If we were using 
> rspinlock_* everywhere then the deadlock (at least in the case when you recurse) 
> would in theory not be possible (unless you recurse too much).
A programming error can lead to a deadlock, yes.

My understanding is that there are deliberate use cases of non-recursive locking
as paths using recursive locking of the same lock are not allowed in those
cases.

Just using rspinlock_* instead of nrspinlock_* would silently ignore such cases,
which is far worse than a deadlock, as those cases might introduce e.g. security
holes.

> My point here is to simplify the interface rather than providing because I don't 
> really see the benefits of providing a non-recursive version for recursive 
> spinlock.
> Anyway as I said this is nothing new.
Correct. Nothing I'd like to address in this series.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature