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

[Zev Weiss <zweiss@equinix.com>]

On Mon, May 10, 2021 at 06:59:24PM CDT, Andrew Jeffery wrote:
>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@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

I guess I'm kind of on the fence on this one, though maybe leaning
slightly in William's direction.  Certainly as Arnd's point illustrates
it's never going to be a totally foolproof safeguard against userspace
that's sufficiently determined to get itself into trouble (e.g. dup(),
fork()), and yes, it does seem fairly analogous to the situation with
ttys...but at the same time, it's not like it compromises the integrity
of the kernel if it does end up happening anyway, and if as your commit
message says multiple-access leads to undefined behavior, it seems like
we might as well make that trap slightly harder to fall into instead of
easier?



Zev