diff mbox series

[5.12,083/161] libceph: dont set global_id until we get an auth ticket

Message ID 20210608175948.243493420@linuxfoundation.org
State New
Headers show
Series None | expand

Commit Message

Greg KH June 8, 2021, 6:26 p.m. UTC
From: Ilya Dryomov <idryomov@gmail.com>

[ 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: stable@vger.kernel.org # needs backporting for < 5.11
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
Reviewed-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 net/ceph/auth.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

Comments

Ilya Dryomov June 8, 2021, 7:07 p.m. UTC | #1
On Tue, Jun 8, 2021 at 8:48 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> From: Ilya Dryomov <idryomov@gmail.com>
>
> [ 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: stable@vger.kernel.org # needs backporting for < 5.11
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> Reviewed-by: Sage Weil <sage@redhat.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  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
Sasha Levin June 8, 2021, 7:24 p.m. UTC | #2
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
><gregkh@linuxfoundation.org> wrote:
>>
>> From: Ilya Dryomov <idryomov@gmail.com>
>>
>> [ 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: stable@vger.kernel.org # needs backporting for < 5.11
>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
>> Reviewed-by: Sage Weil <sage@redhat.com>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>> ---
>>  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.
Ilya Dryomov June 8, 2021, 7:39 p.m. UTC | #3
On Tue, Jun 8, 2021 at 9:24 PM Sasha Levin <sashal@kernel.org> 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
> ><gregkh@linuxfoundation.org> wrote:
> >>
> >> From: Ilya Dryomov <idryomov@gmail.com>
> >>
> >> [ 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: stable@vger.kernel.org # needs backporting for < 5.11
> >> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> >> Reviewed-by: Sage Weil <sage@redhat.com>
> >> Signed-off-by: Sasha Levin <sashal@kernel.org>
> >> ---
> >>  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: <20210608011339.51B0F6124C@mail.kernel.org>
    In-Reply-To: <20210608011339.51B0F6124C@mail.kernel.org>
    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 <idryomov@gmail.com>
    To: Sasha Levin <sashal@kernel.org>
    Cc: stable-commits@vger.kernel.org
    Content-Type: text/plain; charset="UTF-8"

Thanks,

                Ilya
diff mbox series

Patch

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;
 }