Re: [PATCH linux dev-5.10 v3 10/18] ipmi: kcs_bmc: Don't enforce single-open policy in the kernel

[Andrew Jeffery]

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:

https://lore.kernel.org/lkml/CAK8P3a2e3zNqMJSN-LAAjYmy8Gr=wjn5MMDMinxawOWcMgo7Ww@mail.gmail.com/

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

https://tldp.org/HOWTO/Serial-HOWTO-13.html

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))
1e789114.kcs
```

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.

Andrew