ACK/Cmnt: [PATCH 2/2] drm/i915: Synchronize active and retire callbacks

[Andrea Righi]

On Thu, Apr 09, 2020 at 09:59:11AM -0700, Sultan Alsawaf wrote:
> On Thu, Apr 09, 2020 at 06:40:43PM +0200, Andrea Righi wrote:
> > >  	/* After the final retire, the entire struct may be freed */
> > > -	if (ref->retire)
> > > -		ref->retire(ref);
> > > +	if (ref->retire) {
> > > +		if (ref->active) {
> > > +			bool freed = false;
> > > +
> > > +			/* Don't race with the active callback, and avoid UaF */
> > > +			down_write(&ref->rwsem);
> > > +			ref->freed = &freed;
> > 
> > So, we allocate "freed" in the stack and assign to ref->freed, so
> > another thread can potentially access this thread's stack... hm.. I'm
> > not sure if this is a safe practice.
> 
> ref->retire() isn't asynchronous for any of the functions registered to it, so
> no other thread accesses this thread's stack.
> 
> > I'm still trying to understand the logic behind this, sorry if I'm
> > asking something dumb... but why not doing something simple like this:
> > 
> > 	down_write(&ref->rwsem);
> > 	if (ref->retire)
> > 		ref->retire(ref);
> > 	up_write(&ref->rwsem);
> 
> I actually made this mistake with one of my earlier attempts. `ref->retire()`
> will free `ref` on the final put, so putting `up_write(&ref->rwsem);` after it
> unconditionally is a use-after-free. And we have no way to access the reference
> counter that determines when the final put occurs, because it varies by each
> user of the API.
> 
> Plus, all the functions involved in the retirement sequence do not return
> values, so trying to make `ref->retire()` return a boolean that says whether
> `ref` was freed or not would require extensive changes across multiple files.
> 
> > Again, why not a simple:
> > 
> > 	down_read(&ref->rwsem);
> > 	if (!atomic_read(&ref->count) && ref->active)
> > 		err = ref->active(ref);
> > 	up_read(&ref->rwsem);
> 
> No reason to hold a lock when we don't need to. There are some users of this API
> that do not have a retire callback (i.e., `ref->retire` is NULL), so in that
> case we don't need to do any synchronization.
> 
> > > +	bool *freed;
> >
> > Why does freed need to be a pointer? Why not just a bool?
> 
> Because everything inside the `ref` struct can be freed after `ref->retire()`
> executes, so accessing `ref` after that would be a use-after-free.
> 
> Thanks for the review!
> 
> Sultan

After looking many times at the code and your comments this is what I
understand so far:

 - ref->retire() and ref->active() can run concurrently when they are
   both defined and we want to prevent this

 - ref->retire() can destroy ref itself when ref->count becomes 0

 - ref->active() can call itself in a recursive way

 - i915_active_fini() is called immediately before ref is destroyed

 - there are no multiple ref->retire() running at the same time for the
   same ref object (same with ref->active())

The code where we start is very difficult to read (not your fault
obviously) and to make it look nicer I think we would need a complete
redesign, that is not a viable option at the moment.

Assuming all of the above is correct I think your solution is
acceptable, therefore:

Acked-by: Andrea Righi <andrea.righi at canonical.com>

---

Just for fun (if it helps) this is how I would do it: less efficient,
but maybe a little more readable (even if the code is still ugly).

I think the __acquires/__releases annotations are helping a bit to
follow the logic, but in particular I would really like to get rid of
that ref->freed pointer used to access data allocated in current's
stack, that makes me feel a little nervous.

struct i915_active {
...
	struct rb_root tree;
	struct mutex mutex;
	atomic_t count;
	struct rw_semaphore rwsem;
	bool freed;
...
}

void __i915_active_init(struct i915_active *ref,
			int (*active)(struct i915_active *ref),
			void (*retire)(struct i915_active *ref),
			struct lock_class_key *mkey,
			struct lock_class_key *wkey)
{
...
	ref->freed = false;
	__init_rwsem(&ref->rwsem, "i915_active.rwsem", rkey);
...
}

static void
__active_retire(struct i915_active *ref) __acquires(&ref->rwsem)
{
...
	if (ref->retire) {
		bool freed = false;

		/* Don't race with the active callback, and avoid UaF */
		down_write(&ref->rwsem);
		if (atomic_read(&ref->count) == 1) {
			/*
			 * The next call to ref->retire() will free
			 * "ref" on the last put, ref->rwsem will be
			 * released in i915_active_fini(), called
			 * immediately before freeing "ref"
			 */
			ref->freed = true;
			freed = true;
		}
		ref->retire(ref);
		if (!freed)
			up_write(&ref->rwsem);
	}
...
}

int i915_active_acquire(struct i915_active *ref)
{
...
	if (!atomic_read(&ref->count) && ref->active) {
		down_read(&ref->rwsem);
		err = ref->active(ref);
		up_read(&ref->rwsem);
	}
...
}

void i915_active_fini(struct i915_active *ref) __releases(&ref->rwsem)
{
	if (ref->freed)
		up_write(&ref->rwsem);
#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
	debug_active_fini(ref);
	GEM_BUG_ON(!RB_EMPTY_ROOT(&ref->tree));
	GEM_BUG_ON(atomic_read(&ref->count));
	mutex_destroy(&ref->mutex);
#endif
}

Thanks!
-Andrea