Re: Maybe a race condition in net/rds/rdma.c?

[zerons <sironhide0null@gmail.com>]

On 3/11/20 22:35, santosh.shilimkar@oracle.com wrote:
> On 3/10/20 9:48 PM, zerons wrote:
>>
>>
>> On 3/11/20 01:53, santosh.shilimkar@oracle.com wrote:
>>> On 3/6/20 4:11 AM, zerons wrote:
>>>>
>>>>
>>>> On 2/28/20 02:10, santosh.shilimkar@oracle.com wrote:
>>>>>
>>>>>>> On 18 Feb 2020, at 14:13, zerons <sironhide0null@gmail.com> wrote:
>>>>>>>
>>>>>>> Hi, all
>>>>>>>
>>>>>>> In net/rds/rdma.c
>>>>>>> (https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/net/rds/rdma.c?h=v5.5.3*n419__;Iw!!GqivPVa7Brio!OwwQCLtjDsKmhaIz0sfaOVSuC4ai5t5_FgB7yqNExGOCBtACtIGLF61NNJyqSDtIAcGoPg$ ),
>>>>>>> there may be a race condition between rds_rdma_unuse() and rds_free_mr().
>>>>>>>
>>>>> Hmmm.. I didn't see email before in my inbox. Please post questions/patches on netdev in future which is the correct mailing list.
>>>>>
>>>>>>> It seems that this one need some specific devices to run test,
>>>>>>> unfortunately, I don't have any of these.
>>>>>>> I've already sent two emails to the maintainer for help, no response yet,
>>>>>>> (the email address may not be in use).
>>>>>>>
>>>>>>> 0) in rds_recv_incoming_exthdrs(), it calls rds_rdma_unuse() when receive an
>>>>>>> extension header with force=0, if the victim mr does not have RDS_RDMA_USE_ONCE
>>>>>>> flag set, then the mr would stay in the rbtree. Without any lock, it tries to
>>>>>>> call mr->r_trans->sync_mr().
>>>>>>>
>>> MR won't stay in the rbtree with force flag. If the MR is used or
>>> use_once is set in both cases its removed from the tree.
>>> See "if (mr->r_use_once || force)"
>>>
>>
>> Sorry, I may misunderstand. Did you mean that if the MR is *used*,
>> it is removed from the tree with or without the force flag in
>> rds_rdma_unuse(), even when r_use_once is not set?
>>
> Once the MR is being used with use_once semantics it gets removed with or without remote side indicating it via extended header. use_once
> optimization was added later. The base behavior is once the MR is
> used by remote and same information is sent via extended header,
> it gets cleaned up with force flag. Force flag ignores whether
> its marked as used_once or not.
> 

Sorry, I am still confused.

I check the code again. The rds_rdma_unuse() is called in two functions,
rds_recv_incoming_exthdrs() and rds_sendmsg().

In rds_sendmsg(), it calls rds_rdma_unuse() *with* force flag only when
the user included a RDMA_MAP cmsg *and* sendmsg() is failed.

In rds_recv_incoming_exthdrs(), the force is *false*. So we can consider
the rds_rdma_unuse() called *without* force flag.
Then I go check where r_use_once can be set.

__rds_rdma_map()
	rds_get_mr()
		rds_setsockopt()

	rds_get_mr_for_dest()
		rds_setsockopt()

	rds_cmsg_rdma_map()
		rds_cmsg_send()
			rds_sendmsg()

It seems to me that r_use_once is controlled by user applications.

I also wonder if we can ensure that the MR found in rds_rdma_unuse()
gets removed, then "if (mr->r_use_once || force)" doesn't make any sense.

Sorry to keep bothering you with my questions. I wish I had such a device 
that I can test it on.

Best regards,