From: "Andrew Jeffery" <andrew@aj.id.au>
To: "William Kennington" <wak@google.com>
Cc: OpenBMC Development <openbmc@lists.ozlabs.org>
Subject: Re: [PATCH linux dev-5.10 v3 10/18] ipmi: kcs_bmc: Don't enforce single-open policy in the kernel
Date: Tue, 11 May 2021 09:29:24 +0930 [thread overview]
Message-ID: <61f9d748-b4e1-4b64-aaf5-1930cda0206d@www.fastmail.com> (raw)
In-Reply-To: <CAPnigKku-EjOnV9gsmnXzH=XZxSU78iLeccNbsK8k2_4b4UwSg@mail.gmail.com>
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
next prev parent reply other threads:[~2021-05-11 0:00 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-10 6:49 [PATCH linux dev-5.10 v3 00/18] ipmi: Allow raw access to KCS devices Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 01/18] ipmi: kcs_bmc_aspeed: Use of match data to extract KCS properties Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 02/18] ipmi: kcs_bmc: Make status update atomic Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 03/18] ipmi: kcs_bmc: Rename {read, write}_{status, data}() functions Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 04/18] ipmi: kcs_bmc: Split out kcs_bmc_cdev_ipmi Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 05/18] ipmi: kcs_bmc: Turn the driver data-structures inside-out Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 06/18] ipmi: kcs_bmc: Split headers into device and client Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 07/18] ipmi: kcs_bmc: Strip private client data from struct kcs_bmc Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 08/18] ipmi: kcs_bmc: Decouple the IPMI chardev from the core Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 09/18] ipmi: kcs_bmc: Allow clients to control KCS IRQ state Andrew Jeffery
2021-05-14 2:46 ` CS20 KWLiu
2021-05-14 3:03 ` Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 10/18] ipmi: kcs_bmc: Don't enforce single-open policy in the kernel Andrew Jeffery
2021-05-10 8:56 ` William Kennington
2021-05-10 23:59 ` Andrew Jeffery [this message]
2021-05-21 7:43 ` Zev Weiss
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 11/18] ipmi: kcs_bmc: Add serio adaptor Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 12/18] dt-bindings: ipmi: Convert ASPEED KCS binding to schema Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 13/18] dt-bindings: ipmi: Add optional SerIRQ property to ASPEED KCS devices Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 14/18] ipmi: kcs_bmc_aspeed: Implement KCS SerIRQ configuration Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 15/18] ipmi: kcs_bmc_aspeed: Fix IBFIE typo from datasheet Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 16/18] ipmi: kcs_bmc_aspeed: Optionally apply status address Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 17/18] ARM: dts: rainier: Enable KCS channel 2 Andrew Jeffery
2021-05-10 6:49 ` [PATCH linux dev-5.10 v3 18/18] ipmi: kcs_bmc: Add a "raw" character device interface Andrew Jeffery
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=61f9d748-b4e1-4b64-aaf5-1930cda0206d@www.fastmail.com \
--to=andrew@aj.id.au \
--cc=openbmc@lists.ozlabs.org \
--cc=wak@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).