RE: [PATCH v8] ceph: fix slab-use-after-free in have_mon_and_osd_map()

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

Hi Ilya,

On Sun, 2025-10-12 at 22:37 +0200, Ilya Dryomov wrote:
> On Fri, Aug 22, 2025 at 12:52 AM Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> > 
> > 

<skipped>

> > 
> > v8
> > Ilya Dryomov has pointed out that __ceph_open_session()
> > has incorrect logic of two nested loops and checking of
> > client->auth_err could be missed because of it.
> > The logic of __ceph_open_session() has been reworked.
> 
> Hi Slava,
> 
> I was confused for a good bit because the testing branch still had v7.
> I went ahead and dropped it from there.
> 

I decided to finish our discussion before changing anything in testing branch. 

> > 
> > Reported-by: David Howells <dhowells@redhat.com>
> > Signed-off-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > cc: Alex Markuze <amarkuze@redhat.com>
> > cc: Ilya Dryomov <idryomov@gmail.com>
> > cc: Ceph Development <ceph-devel@vger.kernel.org>
> > ---
> >  net/ceph/ceph_common.c | 43 +++++++++++++++++++++++++++++++++++-------
> >  net/ceph/debugfs.c     | 17 +++++++++++++----
> >  net/ceph/mon_client.c  |  2 ++
> >  net/ceph/osd_client.c  |  2 ++
> >  4 files changed, 53 insertions(+), 11 deletions(-)
> > 
> > diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
> > index 4c6441536d55..2a7ca942bc2f 100644
> > --- a/net/ceph/ceph_common.c
> > +++ b/net/ceph/ceph_common.c
> > @@ -790,8 +790,18 @@ EXPORT_SYMBOL(ceph_reset_client_addr);
> >   */
> >  static bool have_mon_and_osd_map(struct ceph_client *client)
> >  {
> > -       return client->monc.monmap && client->monc.monmap->epoch &&
> > -              client->osdc.osdmap && client->osdc.osdmap->epoch;
> > +       bool have_mon_map = false;
> > +       bool have_osd_map = false;
> > +
> > +       mutex_lock(&client->monc.mutex);
> > +       have_mon_map = client->monc.monmap && client->monc.monmap->epoch;
> > +       mutex_unlock(&client->monc.mutex);
> > +
> > +       down_read(&client->osdc.lock);
> > +       have_osd_map = client->osdc.osdmap && client->osdc.osdmap->epoch;
> > +       up_read(&client->osdc.lock);
> > +
> > +       return have_mon_map && have_osd_map;
> >  }
> > 
> >  /*
> > @@ -800,6 +810,7 @@ static bool have_mon_and_osd_map(struct ceph_client *client)
> >  int __ceph_open_session(struct ceph_client *client, unsigned long started)
> >  {
> >         unsigned long timeout = client->options->mount_timeout;
> > +       int auth_err = 0;
> >         long err;
> > 
> >         /* open session, and wait for mon and osd maps */
> > @@ -813,13 +824,31 @@ int __ceph_open_session(struct ceph_client *client, unsigned long started)
> > 
> >                 /* wait */
> >                 dout("mount waiting for mon_map\n");
> > -               err = wait_event_interruptible_timeout(client->auth_wq,
> > -                       have_mon_and_osd_map(client) || (client->auth_err < 0),
> > -                       ceph_timeout_jiffies(timeout));
> > +
> > +               DEFINE_WAIT_FUNC(wait, woken_wake_function);
> > +
> > +               add_wait_queue(&client->auth_wq, &wait);
> > +
> > +               if (!have_mon_and_osd_map(client)) {
> 
> Only half of the original
> 
>     have_mon_and_osd_map(client) || (client->auth_err < 0)
> 
> condition is checked here.  This means that if finish_auth() sets
> client->auth_err and wakes up client->auth_wq before the entry is added
> to the wait queue by add_wait_queue(), that wakeup would be missed.
> The entire condition needs to be checked between add_wait_queue() and
> remove_wait_queue() calls -- anything else is prone to various race
> conditions that lead to hangs.
> 
> > +                       if (signal_pending(current)) {
> > +                               err = -ERESTARTSYS;
> > +                               break;
> 
> If this break is hit, remove_wait_queue() is never called and on top of
> that __ceph_open_session() returns success.  ERESTARTSYS gets suppressed
> and so instead of aborting the opening of the session the code proceeds
> with setting up the debugfs directory and further steps, all with no
> monmap or osdmap received or even potentially in spite of a failure to
> authenticate.
> 

As far as I can see, we are stuck in the discussion. I think it will be more
productive if you can write your own vision of this piece of code. We are still
not on the same page and we can continue this hide and seek game for a long time
yet. Could you please write your vision of this piece of modification?

Thanks,
Slava.