Re: [PATCH 3/3] exec: Transform exec_update_mutex into a rw_semaphore

[Eric W. Biederman]

Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> writes:

> Hi Eric,
>
> I think I remembered from a previous discussion about this topic,
> that it was unclear if the rw_semaphores are working the same
> in RT-Linux.  Will this fix work in RT as well?

The locks should work close enough to the same that correct code is also
correct code on RT-linux.  If not it is an RT-linux bug.

An rw_semaphore may be less than optimal on RT-linux.  I do remember
that mutexes are prefered.  But this change is more about correctness
than anything else.

> On 12/3/20 9:12 PM, Eric W. Biederman wrote:
>> --- a/kernel/kcmp.c
>> +++ b/kernel/kcmp.c
>> @@ -70,25 +70,25 @@ get_file_raw_ptr(struct task_struct *task, unsigned int idx)
>>  	return file;
>>  }
>>  
>> -static void kcmp_unlock(struct mutex *m1, struct mutex *m2)
>> +static void kcmp_unlock(struct rw_semaphore *l1, struct rw_semaphore *l2)
>>  {
>> -	if (likely(m2 != m1))
>> -		mutex_unlock(m2);
>> -	mutex_unlock(m1);
>> +	if (likely(l2 != l1))
>
> is this still necessary ?

Yes.  Both pids could be threads of the same process or even the same
value so yes this is definitely necessary.  rw_semaphores don't nest on
the same cpu.

>
>> +		up_read(l2);
>> +	up_read(l1);
>>  }
>>  
>> -static int kcmp_lock(struct mutex *m1, struct mutex *m2)
>> +static int kcmp_lock(struct rw_semaphore *l1, struct rw_semaphore *l2)
>>  {
>>  	int err;
>>  
>> -	if (m2 > m1)
>> -		swap(m1, m2);
>> +	if (l2 > l1)
>> +		swap(l1, l2);
>
> and this is probably also no longer necessary?

I think lockdep needs this, so it can be certain the same rw_semaphore
is not nesting on the cpu.   Otherwise we will have inconsitencies about
which is the nested lock.  It won't matter in practice, but I am not
certain lockdep knows enough to tell the difference.

If anything removing the swap is a candidate for a follow up patch
where it can be considered separately from other concerns.  For this
patch keeping the logic unchanged makes it trivial to verify that
the conversion from one lock to another is correct.

>>  
>> -	err = mutex_lock_killable(m1);
>> -	if (!err && likely(m1 != m2)) {
>> -		err = mutex_lock_killable_nested(m2, SINGLE_DEPTH_NESTING);
>> +	err = down_read_killable(l1);
>> +	if (!err && likely(l1 != l2)) {
>
> and this can now be unconditionally, right?

Nope.  The two locks can be the same lock, and they don't nest on a
single cpu.  I tested and verified that lockdep complains bitterly
if down_read_killable_nested is replaced with a simple
down_read_killable.


>> +		err = down_read_killable_nested(l2, SINGLE_DEPTH_NESTING);
>>  		if (err)
>> -			mutex_unlock(m1);
>> +			up_read(l1);
>>  	}
>>  
>>  	return err;
>> @@ -156,8 +156,8 @@ SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
>>  	/*
>>  	 * One should have enough rights to inspect task details.
>>  	 */
>> -	ret = kcmp_lock(&task1->signal->exec_update_mutex,
>> -			&task2->signal->exec_update_mutex);
>> +	ret = kcmp_lock(&task1->signal->exec_update_lock,
>> +			&task2->signal->exec_update_lock);
>>  	if (ret)
>>  		goto err;
>>  	if (!ptrace_may_access(task1, PTRACE_MODE_READ_REALCREDS) ||
>> @@ -212,8 +212,8 @@ SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
>>  	}
>>  
>>  err_unlock:
>> -	kcmp_unlock(&task1->signal->exec_update_mutex,
>> -		    &task2->signal->exec_update_mutex);
>> +	kcmp_unlock(&task1->signal->exec_update_lock,
>> +		    &task2->signal->exec_update_lock);
>>  err:
>>  	put_task_struct(task1);
>>  	put_task_struct(task2);
>
>
> Thanks
> Bernd.