libap: please consider stop using liblockfile

[lucab]

This project is currently using liblockfile for file-locking in libap.

That library is mainly targeted at mailbox locking, and it has a footprint which is quite larger than needed. It also comes with a set-gid binary (to escalate privileges to the mail group) which is problematic from the security point of view of downstream distributions. On this topic, there have been some discussion on how to reduce the security impact of this in Fedora at https://github.com/coreos/fedora-coreos-tracker/issues/1253 and https://bugzilla.redhat.com/show_bug.cgi?id=2112857.

Looking through the code here I realized that it is only using the lockfile_create() / lockfile_remove(), and that it doesn't need any of the mailbox-locking logic nor the set-gid binary either.

As such, it would be great if this codebase could stop linking against liblockfile and implement some simpler file-locking logic internally.

How would you feel about that?

[lucab]

I quickly looked into actually patching this out and I think it would be overall feasible in a non-invasive way.

Just as a reference, my current patch for this is at https://github.com/lucab/s390-tools/pull/1. I can submit this if you want, but I'd also be happy if some usual developer could take this ticket and drive it to completion. I don't have any s390x knowledge nor hardware, and I can't actually test this in a meaningful way.

[hoeppnerj]

We're obviously happy to take PRs. But if testing is an issue, we can certainly take your code as a suggestion as well. However, I'm still debating whether we actually want our own locking implementation. If so, it should probably go into libutil rather than libap. I'm also wondering whether flock(2) from util-linux would be an alternative.

[sharkcz]

seems flock is already used by the chreipl-fcp-mpath tools, I think it might be the alternative

[lucab]

However, I'm still debating whether we actually want our own locking implementation.

Having looked at the inner logic of the whole liblockfile library and the minuscule portion that libap effectively uses, my technical feedback is that yes you'd be better served by your own locking logic. I can't really give feedback on how that should look like, possibly hand-rolling a flock-based one is indeed good enough. Overall my patch was just to show that internal locking was feasible without much disruption. I'd be actually happier if you drive this in whatever direction you prefer, and I can scrap my code changes :) I'd rather not venture into a PR for this myself.

[rosatomj]

Hmm, I don't think flock will work.

The current liblockfile usage for s390-tools boils down to:

1) allow chzdev and lszdev to acquire the file lock when doing certain commands related to the 'ap' type. This is straightforward and flock would be a reasonable alternative in this case.

2) allow ap-check to acquire a file lock on behalf of mdevctl (e.g. acquire lock using parent pid). This allows the file lock to persist across multiple mdevctl callout events to ap-check while simultaneously ensuring that stale lock detection uses the parent mdevctl pid (e.g. if mdevctl dies or otherwise has a bug that results in it never calling ap-check that last time to release the file lock, it will be considered stale anyway once the mdevctl pid is gone). IIRC this feature was specifically why liblockfile was chosen.

I don't see anything comparable in flock for this (relies on the file remaining open) so I suspect to retain file locking without liblockfile we would have to roll our own after all.

[dustymabe]

Does https://github.com/lucab/s390-tools/pull/1 satisfy those requirements?

[rosatomj]

Having to incorporate our own file locking is a bit of a bummer, but at a glance I think that yes that PR has the necessary pieces from liblockfile -- though I need to further review/test/verify that. It likely needs some minor tweaking too (e.g. may as well generalize it outside of libap as Jan mentioned above) -- assuming @hoeppnerj is OK with it I can take care of the minor changes on top (and fixes if needed).

[hoeppnerj]

Maybe to ensure everyone gets the full picture. We've now implemented our own locking logic which is provided through libutil/util_lockfile (See also: https://github.com/ibm-s390-linux/s390-tools/commit/e1aec24e843689524b470081fd46e67593161edd) (Thanks @rosatomj ).

Adapting the suggestion (pull request) by @lucab wouldn't be an option for us due to license violations. However, the effort and suggestions were much appreciated nonetheless! Thanks for the input!

[sharkcz]

The final solution looks good to me, thanks to everyone.

[lucab]

Thanks! For our own future reference, the fix for this went into release v2.24.0.