[PATCH] resctrl: Do not open directory for writing

[Andrea Bolognani]

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 [...]

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.

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().

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. 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.

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)

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.

tl;dr

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

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

-- 
Andrea Bolognani / Red Hat / Virtualization