netdev - Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <74255414-7e5c-e490-4745-9a8b9a73488d@oracle.com>
Date:   Tue, 2 Jul 2019 15:12:35 -0700
From:   Gerd Rausch <gerd.rausch@...cle.com>
To:     santosh.shilimkar@...cle.com, netdev@...r.kernel.org
Cc:     David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or
 FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

On 02/07/2019 14.18, santosh.shilimkar@...cle.com wrote:
> On 7/2/19 2:05 PM, Gerd Rausch wrote:
>> What do you call "RDS_GET_MR" semantics?
>>
> Its a blocking socket call. Meaning after this call return to the
> user, the key must be valid. With async registration that can't be
> guaranteed.
> 

If the "IB_WR_REG_MR" operation does not complete successfully within 
the given (to-be-discussed?) timeout, "rds_ib_post_reg_frmr" will return
"-EBUSY".

And that should propagate up the entire stack and make its way into
"setsockopt" returning "-1" with "errno == EBUSY".

Do you see a problem with this approach?
Did you observe a situation where this did not work?

Are you saying that no timeout, no matter how large, is large enough?
If that's the case, we can consider turning the "wait_event_timeout"
into a "wait_event".

>> Are you suggesting to
>> a) Not fix this bug right now and wait until some later point in time
> When did I say that ? I said have you explored alternate approach to
> fix the issue and if not could you try it out.
> 

Why explore an alternate approach?
Do you see a problem with the proposed patch (other than the choice of timeout)?

>> b) Use a different fix. If you've got a different fix, please share.
>>
> I don't but its a review of the fix and possible alternate needs to
> be discussed. It is not like take my fix or provide an alternate fix.
> 

As it is, the upstream implementation of RDS does not work.
IMO, it is desirable to make it work.

If there are future and better implementation of existing functionality
that is fine.
That should not preclude us from fixing what is broken as soon as we can.

>> And besides these options, is there anything wrong with this fix
>> (other than the discussion of what the timeout value ought to be,
>>   which we can address)?
>>
> That timeout is a problem because it doesn't guarantee the failure
> of operation since its an asyn operation for registration.

The fact that the work-request is asynchroneous is precisely what
makes it necessary to wait for the completion before moving on.

That is the proposed change of waiting for the completion
(or a time-out to put an upper bound to the wait)
does.

Replace the "wait_event_timeout" mentally with a "wait_event":
In that case, the process will be stuck in the corresponding function
(e.g. "rds_ib_post_reg_frmr") until the completion handler has occurred
and the "wake_up" call was issued.

It is the job of the "rds_ib_mr_cqe_handler" handler to inspect
the status of the work-completion and set the "fr_state" accordingly.

As far as I can tell, that is happening.

The debate on whether to use a "wait_event" or "wait_event_timeout"
is strictly a debate over whether or not there should
be an upper bound for the firmware to respond.

If there is not: It should be "wait_event".

If there is: It should be "wait_event_timeout", and we need to specify
what that upper bound is.

> Instead of timing out if you poll the CQ for that operation completion, it
> makes it full proof. That is the change Avinash has done iirc and
> am requesting to look at that fix.
> 

The "wake_up" call is issued from within the completion handler.

The completion handler "rds_ib_mr_cqe_handler" is called upon
handling the work-completions coming out of "ib_poll_cq".

That is necessary in order to check the status of the completion.

There are many changes that were done in the Oracle internal repository
(including changes that Avinash had done),
that we will go through in order to see what needs to be fixed in the
upstream version of RDS.

But unless you can see something that is wrong with this proposed fix,
I would suggest we don't leave the upstream RDS broken for extended
periods of time, but rather fix it.

> Other 5 fixes from the series looks fine.
> 

Thank you,

  Gerd