linux-kernel - Re: [RFC PATCH] dm ioctl: fix erroneous EINVAL when signaled

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CACGdZYKbdyALADEMDV+Vg+eog+UjjgGigEpmJTSKw_64RM8rbA@mail.gmail.com>
Date: Wed, 17 Jul 2024 12:52:40 -0700
From: Khazhy Kumykov <khazhy@...omium.org>
To: Mikulas Patocka <mpatocka@...hat.com>
Cc: Alasdair Kergon <agk@...hat.com>, Mike Snitzer <snitzer@...nel.org>, 
	Zdenek Kabelac <zkabelac@...hat.com>, Joe Thornber <thornber@...hat.com>, 
	Heinz Mauelshagen <heinzm@...hat.com>, dm-devel@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] dm ioctl: fix erroneous EINVAL when signaled

On Wed, Jul 17, 2024 at 12:45 PM Mikulas Patocka <mpatocka@...hat.com> wrote:
>
> Hi
>
> I am wondering why does do_resume need to call dm_suspend at all. Does
> anyone here remember why is this code path needed?

In our case, we have a sequence with load_table followed by a resume,
with no suspend first. The resume path suspends if needed, swaps
tables, then resumes. Removing the suspend here would break existing
userspace, I'd imagine. It seems like minimizing the suspended time
would also be a nice benefit.

>
> Mikulas
>
>
>
> On Wed, 17 Jul 2024, Khazhismel Kumykov wrote:
>
> > do_resume when loading a new map first calls dm_suspend, which could
> > silently fail. When we proceeded to dm_swap_table, we would bail out
> > with EINVAL. Instead, restore new_map and return ERESTARTSYS when
> > signaled.
> >
> > Signed-off-by: Khazhismel Kumykov <khazhy@...gle.com>
> > ---
> >  drivers/md/dm-ioctl.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> >
> > RFC as I am rather unfamiliar with the locking semantics here - whether
> > we do need to re-grab hash_lock to write to an hc we previously grabbed,
> > and whether the device becoming unhashed while we're in this function is
> > really something that needs to be checked. However, this patch does fix
> > the issue we were seeing - we'd get EINVAL when thread in ioctl was
> > signaled.
> >
> >
> > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> > index c2c07bfa6471..b81650c6d096 100644
> > --- a/drivers/md/dm-ioctl.c
> > +++ b/drivers/md/dm-ioctl.c
> > @@ -1181,8 +1181,22 @@ static int do_resume(struct dm_ioctl *param)
> >                       suspend_flags &= ~DM_SUSPEND_LOCKFS_FLAG;
> >               if (param->flags & DM_NOFLUSH_FLAG)
> >                       suspend_flags |= DM_SUSPEND_NOFLUSH_FLAG;
> > -             if (!dm_suspended_md(md))
> > -                     dm_suspend(md, suspend_flags);
> > +             if (!dm_suspended_md(md)) {
> > +                     r = dm_suspend(md, suspend_flags);
> > +                     if (r == -EINTR)
> > +                             r = -ERESTARTSYS;
> > +                     if (r) {
> > +                             down_write(&_hash_lock);
> > +                             hc = dm_get_mdptr(md);
> > +                             if (!hc)
> > +                                     r = -ENXIO;
> > +                             else
> > +                                     hc->new_map = new_map;
Oh - I probably want to check if hc->new_map has become non-null in
the meantime and if so... pick a winner then put the loser? Presumably
the newest map should win if that happens / is possible. although the
concept seems suspect to me.
> > +                             up_write(&_hash_lock);
> > +                             dm_put(md);
> > +                             return r;
> > +                     }
> > +             }
> >
> >               old_size = dm_get_size(md);
> >               old_map = dm_swap_table(md, new_map);
> > --
> > 2.45.2.993.g49e7a77208-goog
> >
>