Re: [PATCH linux dev-5.10 v3 10/18] ipmi: kcs_bmc: Don't enforce single-open policy in the kernel
andrew at aj.id.au
Tue May 11 09:59:24 AEST 2021
On Mon, 10 May 2021, at 18:26, William Kennington wrote:
> Why would we want to change this? I personally think the original
> mutual exclusion policy makes even more sense with multiple client
> types, so that the upstack programs actually know when they are being
> locked out. Having clients that are able to open the fd but remain
> broken if they don't do higher level synchronization just feels like a
> good way to have hard to understand behavior.
So there's this from Arnd:
> On Wed, Mar 3, 2021 at 2:54 PM Alex Bennée <alex.bennee at linaro.org> wrote:
> > + /* the rpmb is single open! */
> > + if (test_and_set_bit(RPMB_DEV_OPEN, &rdev->status))
> > + return -EBUSY;
> open counters on device nodes are fundamentally broken, because
> they do not stop you from using dup() or sharing the file descriptor
> across a fork. Just remove this.
Assuming Arnd's point didn't stand on its own, trying to prevent
multiple opens requires two classes of checks in the kernel, where:
1. Multiple file descriptors can be associated with each device node
2. Multiple device nodes can be associated with each device.
Ensuring we don't have multiple-opens via multiple device nodes (2
above) can be done generally in kcs_bmc.c by associating the device
node (client) context with the device driver instance and erroring out
if an association already exists. But addressing 1. requires each
client (chardev) implementation to enforce the 1-fd-per-node
requirement as well, which isn't great.
If you squint, the IPMI KCS devices look like a simple UART and so we
can look to the TTY layer for inspiration. TTYs suffer the same issue
of hard to understand behaviour in the face of multiple opens, and
define a single mechanism for avoiding both 1 and 2 above by way of
userspace lock files:
The lock should be defined in terms of the underlying device as we can
have multiple behaviours exposed through multiple chardevs for each
device. The device can be derived by e.g:
# echo $(basename $(realpath /sys$(udevadm info --query=path /dev/raw-kcs4)/device))
Given that the kernel currently attempts to prevent multiple open we
can assume this is something userspace isn't doing, thus it's safe to
lift the restriction.
If we do have userspace competing for access then it needs to implement
the locking scheme outlined above, which can be done regardless of
whether the kernel supports multiple-open or not.
More information about the openbmc