On Mon, Aug 17, 2020 at 17:40:04 +0200, Michal Privoznik wrote:
> On 8/17/20 5:16 PM, Peter Krempa wrote:
> > On Mon, Aug 17, 2020 at 16:26:54 +0200, Michal Privoznik wrote:
> > > In one of my latest patch (v6.6.0~30) I was trying to remove
> > > libdevmapper use in favor of our own implementation. However, the
> > > code did not take into account that device mapper can be not
> > > compiled into the kernel (e.g. be a separate module that's not
> > > loaded) in which case /proc/devices won't have the device-mapper
> > > major number and thus virDevMapperGetTargets() and/or
> > > virIsDevMapperDevice() fails.
> > >
> > > Fixes: 22494556542c676d1b9e7f1c1f2ea13ac17e1e3e
> > > Reported-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> > > ---
> > > src/util/virdevmapper.c | 41 +++++++++++++++++++++++++++++++----------
> > > 1 file changed, 31 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
> > > index fe7f611496..cc33d8211e 100644
> > > --- a/src/util/virdevmapper.c
> > > +++ b/src/util/virdevmapper.c
> > > @@ -47,10 +47,11 @@
> > > G_STATIC_ASSERT(BUF_SIZE > sizeof(struct dm_ioctl));
> > > static unsigned int virDMMajor;
> > > +static virMutex virDevMapperInitMutex = VIR_MUTEX_INITIALIZER;
> >
> > 'virDMMajor' should now be initialized explicitly ...
>
> Aren't global static variables initialized to zero automatically (something
> about .bss section)?
Yes, they are.
>
> >
> > > static int
> > > -virDevMapperOnceInit(void)
> > > +virDevMapperGetMajor(unsigned int *dmmaj)
> > > {
> > > g_autofree char *buf = NULL;
> > > VIR_AUTOSTRINGLIST lines = NULL;
> > > @@ -69,23 +70,34 @@ virDevMapperOnceInit(void)
> > > if (sscanf(lines[i], "%u %ms\n", &maj, &dev) == 2 &&
> > > STREQ(dev, DM_NAME)) {
> > > - virDMMajor = maj;
> >
> > ... since it's not always initialized here.
> >
> > > + *dmmaj = maj;
> > > break;
> > > }
> > > }
> > > if (!lines[i]) {
> > > - virReportError(VIR_ERR_INTERNAL_ERROR,
> > > - _("Unable to find major for %s"),
> > > - DM_NAME);
> > > - return -1;
> > > + /* Don't error here. Maybe the kernel was built without
> > > + * devmapper. Let the caller decide. */
> > > + return -2;
> > > }
> > > return 0;
> > > }
> > > -VIR_ONCE_GLOBAL_INIT(virDevMapper);
> > > +static int
> > > +virDevMapperInit(void)
> > > +{
> > > + int rc = 0;
> > > +
> > > + virMutexLock(&virDevMapperInitMutex);
> > > +
> > > + if (virDMMajor == 0)
> >
> > I'm not quite persuaded with this caching here. For cases when the
> > device mapper is loaded we cache it, but in the negative case ...
> >
> > > + rc = virDevMapperGetMajor(&virDMMajor);
> >
> > ... we always process /proc/devices. Why is caching necessary then?
>
> Are you suggesting to parse /proc/devices every time? My concern is it will
> burn CPU cycles needlessly. And I'm not sure how to address the negative
> case when DM module is not loaded. It's ineffective, I agree, I just don't
> see how to make it better.
Well, at first we should establish when the value is determined/can
change:
1) at kernel build time/boot time (any case when the libvirtd process
will need to be restarted for it to change)
In these scenarios it doesn't actually make sense to check it prior to
trying to open the device mapper control socket first. You can look it
up and cache it after you open the socket and don't ever need to re-do
it. In that case you can also use the existing VIR_ONCE code.
You then don't have to clear it when the module is ejected, because
afterwards the control socket will not exist. In case the module is
re-injected, given the contstraints above the value will not change so
no need to re-load.
2) at module insertion time
In this case it's actually wrong to cache it, because the module can
be unloaded and reloaded while libvirt will not check and update the
cached value. In those scenarios it should also be determined only
after you open the control fd frist.
>
> >
> > > +
> > > + virMutexUnlock(&virDevMapperInitMutex);
> > > + return rc;
> > > +}
> > > static void *
> > > @@ -220,7 +232,6 @@ virDevMapperGetTargetsImpl(int controlFD,
> > > size_t i;
> > > memset(&dm, 0, sizeof(dm));
> > > - *devPaths_ret = NULL;
> > > if (ttl == 0) {
> > > errno = ELOOP;
> > > @@ -298,14 +309,24 @@ virDevMapperGetTargets(const char *path,
> > > char ***devPaths)
> > > {
> > > VIR_AUTOCLOSE controlFD = -1;
> > > + int rc;
> > > const unsigned int ttl = 32;
> > > /* Arbitrary limit on recursion level. A devmapper target can
> > > * consist of devices or yet another targets. If that's the
> > > * case, we have to stop recursion somewhere. */
> > > - if (virDevMapperInitialize() < 0)
> > > + *devPaths = NULL;
> > > +
> > > + if ((rc = virDevMapperInit()) < 0) {
> > > + if (rc == -2) {
> > > + /* Devmapper is not available. Yet. Maybe the module
> > > + * will be loaded later, but do not error out for now. */
> > > + return 0;
> > > + }
> > > +
> > > return -1;
> > > + }
> > > if ((controlFD = virDMOpen()) < 0)
> > > return -1;
> > > @@ -319,7 +340,7 @@ virIsDevMapperDevice(const char *dev_name)
> > > {
> > > struct stat buf;
> > > - if (virDevMapperInitialize() < 0)
> > > + if (virDevMapperInit() < 0)
> > > return false;
> > > if (!stat(dev_name, &buf) &&
> >
> > Code after this hunk reads 'virDMMajor' directly without locks. Since
> > virDevMapperInit returns it here it should be used from the return value
> > rather than accessed directly which creates a code-pattern open for race
> > conditions.
>
> I'm not sure I follow. virDevMapperInit() returns -2, -1, or 0. Not the
> major. And reading without lock is fine because there is no way that the
Sorry, I thought it returned it, which would IMO make sense given the
locks.
> virDMMajor is not set at this point (in which case the virDevMapperInit() is
> basically a NOP. But if it makes us feel better I can put some locking
> around.
Well, right in the next patch you introduce a way which can change the
value without the lock to 0.
I'm not saying that the code will break, but you are adding code which
is not obvious without properly documenting the implications and
contstraints in the code.
Once somebody else will try to see how it works, this will most
certainly look wrong on the first glance given the obvius races present
even when they are impossible due to constraints outside of libvirt.
