| Message ID | 20210608175948.243493420@linuxfoundation.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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.
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 --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; }