On Tue, Jun 8, 2021 at 8:48 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> From: Ilya Dryomov <[email protected]>
>
> [ Upstream commit 61ca49a9105faefa003b37542cebad8722f8ae22 ]
>
> With the introduction of enforcing mode, setting global_id as soon
> as we get it in the first MAuth reply will result in EACCES if the
> connection is reset before we get the second MAuth reply containing
> an auth ticket -- because on retry we would attempt to reclaim that
> global_id with no auth ticket at hand.
>
> Neither ceph_auth_client nor ceph_mon_client depend on global_id
> being set ealy, so just delay the setting until we get and process
> the second MAuth reply. While at it, complain if the monitor sends
> a zero global_id or changes our global_id as the session is likely
> to fail after that.
>
> Cc: [email protected] # needs backporting for < 5.11
> Signed-off-by: Ilya Dryomov <[email protected]>
> Reviewed-by: Sage Weil <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>
> ---
> net/ceph/auth.c | 36 +++++++++++++++++++++++-------------
> 1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/net/ceph/auth.c b/net/ceph/auth.c
> index eb261aa5fe18..de407e8feb97 100644
> --- a/net/ceph/auth.c
> +++ b/net/ceph/auth.c
> @@ -36,6 +36,20 @@ static int init_protocol(struct ceph_auth_client *ac, int proto)
> }
> }
>
> +static void set_global_id(struct ceph_auth_client *ac, u64 global_id)
> +{
> + dout("%s global_id %llu\n", __func__, global_id);
> +
> + if (!global_id)
> + pr_err("got zero global_id\n");
> +
> + if (ac->global_id && global_id != ac->global_id)
> + pr_err("global_id changed from %llu to %llu\n", ac->global_id,
> + global_id);
> +
> + ac->global_id = global_id;
> +}
> +
> /*
> * setup, teardown.
> */
> @@ -222,11 +236,6 @@ int ceph_handle_auth_reply(struct ceph_auth_client *ac,
>
> payload_end = payload + payload_len;
>
> - if (global_id && ac->global_id != global_id) {
> - dout(" set global_id %lld -> %lld\n", ac->global_id, global_id);
> - ac->global_id = global_id;
> - }
> -
> if (ac->negotiating) {
> /* server does not support our protocols? */
> if (!protocol && result < 0) {
> @@ -253,11 +262,16 @@ int ceph_handle_auth_reply(struct ceph_auth_client *ac,
>
> ret = ac->ops->handle_reply(ac, result, payload, payload_end,
> NULL, NULL, NULL, NULL);
> - if (ret == -EAGAIN)
> + if (ret == -EAGAIN) {
> ret = build_request(ac, true, reply_buf, reply_len);
> - else if (ret)
> + goto out;
> + } else if (ret) {
> pr_err("auth protocol '%s' mauth authentication failed: %d\n",
> ceph_auth_proto_name(ac->protocol), result);
> + goto out;
> + }
> +
> + set_global_id(ac, global_id);
>
> out:
> mutex_unlock(&ac->mutex);
> @@ -484,15 +498,11 @@ int ceph_auth_handle_reply_done(struct ceph_auth_client *ac,
> int ret;
>
> mutex_lock(&ac->mutex);
> - if (global_id && ac->global_id != global_id) {
> - dout("%s global_id %llu -> %llu\n", __func__, ac->global_id,
> - global_id);
> - ac->global_id = global_id;
> - }
> -
> ret = ac->ops->handle_reply(ac, 0, reply, reply + reply_len,
> session_key, session_key_len,
> con_secret, con_secret_len);
> + if (!ret)
> + set_global_id(ac, global_id);
> mutex_unlock(&ac->mutex);
> return ret;
> }
Hi Greg,
I asked Sasha to drop this patch earlier today.
Thanks,
Ilya
On Tue, Jun 08, 2021 at 09:07:18PM +0200, Ilya Dryomov wrote:
>On Tue, Jun 8, 2021 at 8:48 PM Greg Kroah-Hartman
><[email protected]> wrote:
>>
>> From: Ilya Dryomov <[email protected]>
>>
>> [ Upstream commit 61ca49a9105faefa003b37542cebad8722f8ae22 ]
>>
>> With the introduction of enforcing mode, setting global_id as soon
>> as we get it in the first MAuth reply will result in EACCES if the
>> connection is reset before we get the second MAuth reply containing
>> an auth ticket -- because on retry we would attempt to reclaim that
>> global_id with no auth ticket at hand.
>>
>> Neither ceph_auth_client nor ceph_mon_client depend on global_id
>> being set ealy, so just delay the setting until we get and process
>> the second MAuth reply. While at it, complain if the monitor sends
>> a zero global_id or changes our global_id as the session is likely
>> to fail after that.
>>
>> Cc: [email protected] # needs backporting for < 5.11
>> Signed-off-by: Ilya Dryomov <[email protected]>
>> Reviewed-by: Sage Weil <[email protected]>
>> Signed-off-by: Sasha Levin <[email protected]>
>> ---
>> net/ceph/auth.c | 36 +++++++++++++++++++++++-------------
>> 1 file changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/net/ceph/auth.c b/net/ceph/auth.c
>> index eb261aa5fe18..de407e8feb97 100644
>> --- a/net/ceph/auth.c
>> +++ b/net/ceph/auth.c
>> @@ -36,6 +36,20 @@ static int init_protocol(struct ceph_auth_client *ac, int proto)
>> }
>> }
>>
>> +static void set_global_id(struct ceph_auth_client *ac, u64 global_id)
>> +{
>> + dout("%s global_id %llu\n", __func__, global_id);
>> +
>> + if (!global_id)
>> + pr_err("got zero global_id\n");
>> +
>> + if (ac->global_id && global_id != ac->global_id)
>> + pr_err("global_id changed from %llu to %llu\n", ac->global_id,
>> + global_id);
>> +
>> + ac->global_id = global_id;
>> +}
>> +
>> /*
>> * setup, teardown.
>> */
>> @@ -222,11 +236,6 @@ int ceph_handle_auth_reply(struct ceph_auth_client *ac,
>>
>> payload_end = payload + payload_len;
>>
>> - if (global_id && ac->global_id != global_id) {
>> - dout(" set global_id %lld -> %lld\n", ac->global_id, global_id);
>> - ac->global_id = global_id;
>> - }
>> -
>> if (ac->negotiating) {
>> /* server does not support our protocols? */
>> if (!protocol && result < 0) {
>> @@ -253,11 +262,16 @@ int ceph_handle_auth_reply(struct ceph_auth_client *ac,
>>
>> ret = ac->ops->handle_reply(ac, result, payload, payload_end,
>> NULL, NULL, NULL, NULL);
>> - if (ret == -EAGAIN)
>> + if (ret == -EAGAIN) {
>> ret = build_request(ac, true, reply_buf, reply_len);
>> - else if (ret)
>> + goto out;
>> + } else if (ret) {
>> pr_err("auth protocol '%s' mauth authentication failed: %d\n",
>> ceph_auth_proto_name(ac->protocol), result);
>> + goto out;
>> + }
>> +
>> + set_global_id(ac, global_id);
>>
>> out:
>> mutex_unlock(&ac->mutex);
>> @@ -484,15 +498,11 @@ int ceph_auth_handle_reply_done(struct ceph_auth_client *ac,
>> int ret;
>>
>> mutex_lock(&ac->mutex);
>> - if (global_id && ac->global_id != global_id) {
>> - dout("%s global_id %llu -> %llu\n", __func__, ac->global_id,
>> - global_id);
>> - ac->global_id = global_id;
>> - }
>> -
>> ret = ac->ops->handle_reply(ac, 0, reply, reply + reply_len,
>> session_key, session_key_len,
>> con_secret, con_secret_len);
>> + if (!ret)
>> + set_global_id(ac, global_id);
>> mutex_unlock(&ac->mutex);
>> return ret;
>> }
>
>Hi Greg,
>
>I asked Sasha to drop this patch earlier today.
I've dropped it now, but I think I'm missing your previous request. Was
it as a reply to the added-to mail? I just want to make sure I'm not
missing your mails.
--
Thanks,
Sasha
On Tue, Jun 8, 2021 at 9:24 PM Sasha Levin <[email protected]> wrote:
>
> On Tue, Jun 08, 2021 at 09:07:18PM +0200, Ilya Dryomov wrote:
> >On Tue, Jun 8, 2021 at 8:48 PM Greg Kroah-Hartman
> ><[email protected]> wrote:
> >>
> >> From: Ilya Dryomov <[email protected]>
> >>
> >> [ Upstream commit 61ca49a9105faefa003b37542cebad8722f8ae22 ]
> >>
> >> With the introduction of enforcing mode, setting global_id as soon
> >> as we get it in the first MAuth reply will result in EACCES if the
> >> connection is reset before we get the second MAuth reply containing
> >> an auth ticket -- because on retry we would attempt to reclaim that
> >> global_id with no auth ticket at hand.
> >>
> >> Neither ceph_auth_client nor ceph_mon_client depend on global_id
> >> being set ealy, so just delay the setting until we get and process
> >> the second MAuth reply. While at it, complain if the monitor sends
> >> a zero global_id or changes our global_id as the session is likely
> >> to fail after that.
> >>
> >> Cc: [email protected] # needs backporting for < 5.11
> >> Signed-off-by: Ilya Dryomov <[email protected]>
> >> Reviewed-by: Sage Weil <[email protected]>
> >> Signed-off-by: Sasha Levin <[email protected]>
> >> ---
> >> net/ceph/auth.c | 36 +++++++++++++++++++++++-------------
> >> 1 file changed, 23 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/net/ceph/auth.c b/net/ceph/auth.c
> >> index eb261aa5fe18..de407e8feb97 100644
> >> --- a/net/ceph/auth.c
> >> +++ b/net/ceph/auth.c
> >> @@ -36,6 +36,20 @@ static int init_protocol(struct ceph_auth_client *ac, int proto)
> >> }
> >> }
> >>
> >> +static void set_global_id(struct ceph_auth_client *ac, u64 global_id)
> >> +{
> >> + dout("%s global_id %llu\n", __func__, global_id);
> >> +
> >> + if (!global_id)
> >> + pr_err("got zero global_id\n");
> >> +
> >> + if (ac->global_id && global_id != ac->global_id)
> >> + pr_err("global_id changed from %llu to %llu\n", ac->global_id,
> >> + global_id);
> >> +
> >> + ac->global_id = global_id;
> >> +}
> >> +
> >> /*
> >> * setup, teardown.
> >> */
> >> @@ -222,11 +236,6 @@ int ceph_handle_auth_reply(struct ceph_auth_client *ac,
> >>
> >> payload_end = payload + payload_len;
> >>
> >> - if (global_id && ac->global_id != global_id) {
> >> - dout(" set global_id %lld -> %lld\n", ac->global_id, global_id);
> >> - ac->global_id = global_id;
> >> - }
> >> -
> >> if (ac->negotiating) {
> >> /* server does not support our protocols? */
> >> if (!protocol && result < 0) {
> >> @@ -253,11 +262,16 @@ int ceph_handle_auth_reply(struct ceph_auth_client *ac,
> >>
> >> ret = ac->ops->handle_reply(ac, result, payload, payload_end,
> >> NULL, NULL, NULL, NULL);
> >> - if (ret == -EAGAIN)
> >> + if (ret == -EAGAIN) {
> >> ret = build_request(ac, true, reply_buf, reply_len);
> >> - else if (ret)
> >> + goto out;
> >> + } else if (ret) {
> >> pr_err("auth protocol '%s' mauth authentication failed: %d\n",
> >> ceph_auth_proto_name(ac->protocol), result);
> >> + goto out;
> >> + }
> >> +
> >> + set_global_id(ac, global_id);
> >>
> >> out:
> >> mutex_unlock(&ac->mutex);
> >> @@ -484,15 +498,11 @@ int ceph_auth_handle_reply_done(struct ceph_auth_client *ac,
> >> int ret;
> >>
> >> mutex_lock(&ac->mutex);
> >> - if (global_id && ac->global_id != global_id) {
> >> - dout("%s global_id %llu -> %llu\n", __func__, ac->global_id,
> >> - global_id);
> >> - ac->global_id = global_id;
> >> - }
> >> -
> >> ret = ac->ops->handle_reply(ac, 0, reply, reply + reply_len,
> >> session_key, session_key_len,
> >> con_secret, con_secret_len);
> >> + if (!ret)
> >> + set_global_id(ac, global_id);
> >> mutex_unlock(&ac->mutex);
> >> return ret;
> >> }
> >
> >Hi Greg,
> >
> >I asked Sasha to drop this patch earlier today.
>
> I've dropped it now, but I think I'm missing your previous request. Was
> it as a reply to the added-to mail? I just want to make sure I'm not
> missing your mails.
Yes, but it looks like it didn't make it to stable-commits mailing list
either. Weird...
MIME-Version: 1.0
Date: Tue, 8 Jun 2021 11:13:08 +0200
References: <[email protected]>
In-Reply-To: <[email protected]>
Message-ID:
<CAOi1vP9Ubs1Cu6sW43H-=dVXSzkFZBycfR_Af4b3vJ9mihkAzA@mail.gmail.com>
Subject: Re: Patch "libceph: don't set global_id until we get an
auth ticket" has been added to the 5.12-stable tree
From: Ilya Dryomov <[email protected]>
To: Sasha Levin <[email protected]>
Cc: [email protected]
Content-Type: text/plain; charset="UTF-8"
Thanks,
Ilya