Date: Sat, 31 Aug 2019 11:27:50 -0000 From: Bruce Evans <brde@optusnet.com.au> To: Warner Losh <imp@bsdimp.com> Cc: John Baldwin <jhb@freebsd.org>, Eugene Grosbein <eugen@grosbein.net>, Bruce Evans <brde@optusnet.com.au>, Freebsd hackers list <freebsd-hackers@freebsd.org> Subject: Re: FreeBSD Security Advisory FreeBSD-SA-19:23.midi Message-ID: <20190831194355.W930@besplex.bde.org> In-Reply-To: <CANCZdfpFPO-a-QJsLc_qHU9W5JKpWmk1NHS6Ek0BpvrDTv9=Jw@mail.gmail.com> References: <20190820201257.7A9D41F8B7@freefall.freebsd.org> <f19d3f62-940c-7888-b379-f416dfc45cac@grosbein.net> <20190830133855.W1405@besplex.bde.org> <5961a5af-d36c-4b8f-20c1-e7054b0149f4@grosbein.net> <a3977237-3f24-ecb9-4399-c8d8fedc26ea@FreeBSD.org> <CANCZdfpFPO-a-QJsLc_qHU9W5JKpWmk1NHS6Ek0BpvrDTv9=Jw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 30 Aug 2019, Warner Losh wrote:
> On Fri, Aug 30, 2019 at 10:06 AM John Baldwin <jhb@freebsd.org> wrote:
>
>> On 8/30/19 12:56 AM, Eugene Grosbein wrote:
>>> 30.08.2019 11:03, Bruce Evans wrote:
>>>
>>>> The patch preserves some historical mistakes and adds some excessive
>>>> verboseness about probe failures. I'm still waiting for jhb to reply to
>>>> mails on 30 Oct 2015 and 23 Jan 2018 asking for a review of this patch
>>>> or better a complete fix.
>>>
>>> Hmm... Maybe additional notification worth it :-)
>>
>> Hmm, I've used 'hint.foo.0.disabled=1' with many devices and it works fine.
>> devctl enable can "undo" the disable post-boot even.
>>
>> It doesn't disable probing, only attaching once we know which driver a
>> device
>> is going to use.
It doesn't work fine. I notice the problem mainly for sio vs uart (I
statically configure lower quality drivers like uart and vt together
with the drivers that I use for regression testing, and select the one
to use by dynamic hints).
sio and uart have hard-coded precedences. These usually select the
wrong driver when neither is disabled. I forget if this is a problem
if 1 of the drivers is disabled. It shouldn't be.
sio's probe is still naughty and depends on setting up some state for sio's
attach (as needed to support old isa irqs not being shareable, but that was
broken before FreeBSD-4). So its probe is far from idempotent. uart's
probe is not idempotent either -- see below.
>> As far as I can tell, the patch makes it disable
>> DEVICE_PROBE as well, but the vast majority of DEVICE_PROBE routines are
>> idempotent.
The patch has a flag variable which may be used to keep some of the probing
benahviour.
But the vast majority of probe routines are not idempotent. They change
the hardware state, not just the softc. I don't remember any even attempting
to restore the hardware state at the end of probes. Console drivers give
further complications -- see below.
>> Only a handful of legacy ISA drivers use 'return (0)' to try
>> to
>> pass state from probe to attach via the softc. Those drivers could choose
>> to
>> check the disabled flag in their probe routine and/or be fixed to have
>> idempotent probe routines. I think the latter is probably the best path
>> forward.
sio is one of these.
The problem is clearest for console drivers. Suppose sio and uart are
both statically configured. One or both must be disabled as a console
driver, else console initialzation of each spamds the one that was
inititalized first. Normal probe/attach is not involved for this, but
the same disable flag is used for this as fr probe/attach. This is not
sufficient, due to the probe clobbering the console state, at least
transiently. In -current:
(1) suppose sio console is initially enabled and uart console is
iniitially disabled. Then:
- sio attaches as console
- uart probe clobbers sio hardware state transiently and on completion
- however, sio console doesn't use the ambient sio hardware state --
it switches the hardware state to a nearly-invariant console state,
as needed to single-step through higher level state changes in sio
via tcsetattr(); this works for adverse probes too.
(2) suppose sio console is initially disabled and uart console is
iniitially endabled. Then:
- uart attaches as console
- sio probe clobbers sio hardware state transiently and on completion
- uart console does no context switching, so uart crashes even single-
stepping its own tcsetattr(). It might work accidetally after sio
probe completes.
> The problem with disabling before PROBE is that if you have N foo devices,
> hint.foo.0.disabled=1 will disable all of them as the probe routines all
> return 'not me' and we try foo0 on each new instance... I'm pretty sure
> that's why it's not done today and why I didn't do it when I added this
> feature because how do you know you have foo0 until probe says 'yup, that's
> mine'?
>
> Most of the old ISA routines that returned 0 I think have been deleted.
> Maybe all since they were for fussy hardware from before the plug and play
> era...
There are still problems with devices being probed and attached on different
buses. On x86, acpi is probed first and isa last (pnp probes naybe first,
but rarey succeed). In one of my hardware configurations, IIRC the same
hardware device is attached as uart2 on puc on acpi (or is it pci) and as
sio4 on puc on acpi/pci unless it is fully disabled (in the probe too).
This device works better as memory mapped, but I only committed the i/o
mapped BARs for it in puc. uart hangs in the probe on the i/o mapped BARs
since the device also needs regshift = 2 and support for regshift != 0 was
axed from puc. puc has remarkably few memory mapped BARs in it, else it
would hamg more. sio only supports i/o mapped BARs except in my uncomitted
version. uart supports memory mapped BARs in puc, but only ones with
regshift=0. The BAR type is determined dynamically, and the problem is
mostly avoided by omitting memory-mapped BARs in puc data.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190831194355.W930>