[PATCH] resctrl: Do not open directory for writing

[Martin Kletzander]

On Thu, Jul 09, 2020 at 12:50:38PM +0200, Andrea Bolognani wrote:
>On Thu, 2020-07-09 at 11:40 +0200, Martin Kletzander wrote:
>>  static int
>>  virResctrlLockWrite(void)
>>  {
>> -    int fd = open(SYSFS_RESCTRL_PATH, O_RDWR | O_CLOEXEC);
>> +    int fd = open(SYSFS_RESCTRL_PATH, O_RDONLY | O_CLOEXEC);
>
>I got curious, so I did some digging.
>
>The commit message for 18dca21a32e9 says
>
>  The O_DIRECTORY flag causes open() to return an error if the
>  filename is a directory. There's no obvious reason why resctrl
>  needs to use this [...]
>

I think that was a typo, yes.

>but open(2) claims
>
>  O_DIRECTORY
>    If pathname is not a directory, cause the open to fail.
>
>instead, so using O_DIRECTORY in the first place doesn't seem at all
>unreasonable: the resctrl mount path *is* going to be a directory. I
>guess we don't need to be too paranoid about it, though, so I don't
>think removing the flag is a big issue.
>

The main reason for removing it was that it is not available on other platforms,
I think it is linux-only and gnulib was handling other platforms for us.  We
don't really care if it is a directory or not.  The very next read somewhere in
that path will fail if it is not a directory.  We only need to make sure we use
flock(2) as that is the lock used for mutual exclusion on resctrl.  Yes, it is
Linux-only, just as O_DIRECTORY, and same as resctrl itself.  At least to my
knowledge.  We could also wrap it in an #ifdef clause, but there is no need for
it, really.  virFileFlock() will currently fail with ENOSYS for non-Linux (or
non-__linux__, to be overly specific).

>Now, when it comes to opening R/O or R/W, I agree that using the
>latter it was not the right choice and it altered the behavior, but
>I assume the decision was influenced by the name of the function,
>virResctrlLockWrite().
>

That could've been, although it refers to a write (exclusive) lock.

>I looked back through the file's history to see whether that name was
>justified by virResctrlLockRead() existing at some point, but that
>doesn't seem to be the case.

So, funny[0] story, there was.  It was just never committed because i could not
use it in the end.  The only usage would be for reporting some information in
capabilities for example.  Since flock(2) does not have lock promotion (and it's
always an issue anyway) reading information has to use a write lock if we are
going to write something based on the values we read, otherwise the data we
would base the settings on could change in the meantime.

It's not a proof, but you can see that by looking up virResctrlLockInternal()
which used to be the function deduplicating same code between read and write
locking.

>I guess the name was inspired by virRWLockWrite()? But since there's no "read"
>equivalent, it seems that the simpler virResctrlLock() would have worked just
>as fine. But I digress.
>

It should be just Lock, no Write, it's confusing, I agree.

>The more interesting thing I noticed is that in 657ddeff2313, the
>locking call was changed from
>
>  flock(fd, LOCK_EX)
>
>to
>
>  virFileFlock(fd, true, true)
>

Yes, that was part of a clean-up that did not get in as a whole, IIRC.  Further
information available only on in-person requests and enough alcohol nearby[1].

>and given the documentation for virFileFlock()
>
>  /**
>   * virFileFlock:
>   * @fd: file descriptor to call flock on
>   * @lock: true for lock, false for unlock
>   * @shared: true if shared, false for exclusive, ignored if
>   *          `@lock == false`
>   *
>   * This is just a simple wrapper around flock(2) that errors out
>   * on unsupported platforms.
>   *
>   * The lock will be released when @fd is closed or this function
>   * is called with `@lock == false`.
>   *
>   * Returns 0 on success, -1 otherwise (with errno set)
>   */
>  int virFileFlock(int fd, bool lock, bool shared)
>
>it seems to me that this change entirely flipped the semantics of
>the lock.
>

Yep, you're right.  When you have a lock that has boolean as a parameter I think
the boolean should indicate whether the lock is writable, but maybe the proper
and most readable solution would be virFileFlockShareable() and
virFileFlockExclusive() to avoid any misconception in the future[2].  Given the
fact that there are no (and most probably won't be) any other users of flock(2)
and given the fact that resctrl is also pretty niche feature, I don't have any
preference.  Also I feel like after some time I'm a little bit rusty with C and
the libvirt codebase (and most importantly the reasoning and decisions) has
changed a bit, so I'll leave the decision on how to deal with that on someone
else.  I'm happy to provide the patches when clear decision is made.

>tl;dr
>
>  Reviewed-by: Andrea Bolognani <abologna at redhat.com>
>

Thanks, I'm pushing this one (let's see if I screw it up or not) and will
prepare for the next ones.

>for this patch, but we probably need to change the second argument
>of the virFileFlock() call and maybe consider renaming the function.
>

[0] not really funny at all
[1] drink responsibly or not at all
[2] well, apart from having a proper sum type and the flock function only
     accepting Flock::Exclusive or Flock::Shared for that parameter or something
     along those lines ;-)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200709/5389135d/attachment-0001.sig>