Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names

Hi,

On Tue, Aug 25, 2020 at 3:54 PM Rob Clark [off-list ref] wrote:

On Tue, Aug 25, 2020 at 3:23 PM Doug Anderson [off-list ref] wrote:

quoted

Hi,

On Tue, Aug 25, 2020 at 12:01 PM Sai Prakash Ranjan
[off-list ref] wrote:

quoted

Hi,

On 2020-08-25 21:40, Doug Anderson wrote:

quoted

Hi,

On Tue, Aug 25, 2020 at 8:43 AM Sai Prakash Ranjan
[off-list ref] wrote:

quoted

Currently the non-strict or lazy mode of TLB invalidation can only be
set
for all or no domains. This works well for development platforms where
setting to non-strict/lazy mode is fine for performance reasons but on
production devices, we need a more fine grained control to allow only
certain peripherals to support this mode where we can be sure that it
is
safe. So add support to filter non-strict/lazy mode based on the
device
names that are passed via cmdline parameter "iommu.nonstrict_device".

Example:
iommu.nonstrict_device="7c4000.sdhci,a600000.dwc3,6048000.etr"

Just curious: are device names like this really guaranteed to be
stable across versions?

quoted

quoted

I have an inherent dislike of jamming things like this onto the
command line.  IMHO the command line is the last resort for specifying
configuration and generally should be limited to some specialized
debug options and cases where the person running the kernel needs to
override a config that was set by the person (or company) compiling
the kernel.  Specifically, having a long/unwieldy command line makes
it harder to use for the case when an end user actually wants to use
it to override something.  It's also just another place to look for
config.

Good thing about command line parameters are that they are optional,
they do
not specify any default behaviour (I mean they are not mandatory to be
set
for the system to be functional), so I would like to view it as an
optional
config. And this command line parameter (nonstrict_device) is strictly
optional
with default being strict already set in the driver.

They can be passed from the bootloader via chosen node for DT platforms
or choose
a new *bootconfig* as a way to pass the cmdline but finally it does boil
down to
just another config.

Never looked at bootconfig.  Unfortunately it seems to require
initramfs so that pretty much means it's out for my usage.  :(

quoted

I agree with general boolean or single value command line parameters
being just
more messy which could just be Kconfigs instead but for multiple value
parameters
like these do not fit in Kconfig.

As you might already know, command line also gives an advantage to the
end user
to configure system without building kernel, for this specific command
line its
very useful because the performance bump is quite noticeable when the
iommu.strict
is off. Now for end user who would not be interested in building entire
kernel(majority)
and just cares about good speeds or throughput can find this very
beneficial.
I am not talking about one specific OS usecase here but more in general
term.

quoted

The other problem is that this doesn't necessarily scale very well.
While it works OK for embedded cases it doesn't work terribly well for
distributions.  I know that in an out-of-band thread you indicated
that it doesn't break anything that's not already broken (AKA this
doesn't fix the distro case but it doesn't make it worse), it would be
better to come up with a more universal solution.

Is the universal solution here referring to fix all the command line
parameters
in the kernel or this specific command line? Are we going to remove any
more
addition to the cmdline ;)

There are very few cases where a kernel command line parameter is the
only way to configure something.  Most of the time it's just there to
override a config.  I wouldn't suggest removing those.  I just don't
want a kernel command line parameter to be the primary way to enable
something.

quoted

So possible other solution is the *bootconfig* which is again just
another place
to look for a config. So thing is that this universal solution would
result in
just more new fancy ways of passing configs or adding such configs to
the drivers
or subsystems in kernel which is pretty much similar to implementing
policy in
kernel which I think is frowned upon and mentioned in the other thread.

quoted

Ideally it feels like we should figure out how to tag devices in a
generic manner automatically (hardcode at the driver or in the device
tree).  I think the out-of-band discussions talked about "external
facing" and the like.  We could also, perhaps, tag devices that have
"binary blob" firmware if we wanted.  Then we'd have a policy (set by
Kconfig, perhaps overridable via commandline) that indicated the
strictness level for the various classes of devices.  So policy would
be decided by KConfig and/or command line.

How is tagging in driver or device tree better than the simple command
line
approach to pass the same list of devices which otherwise you would
hardcode
in the corresponding drivers and device tree all over the kernel other
than
the scalability part for command line? IMHO it is too much churn.

It's better because it doesn't require keeping track and updating
these per-board (or per machine) arguments for each and every
board/machine you maintain.  If, for instance, we start out by
allowing HW video decoder to use non-strict.  So:

On one board, we add in "aa00000.video-codec" to the command line.
On some other board, maybe we add in "1d00000.video-codec" to the command line.
On some other board, maybe we add in "90400000.video-codec" to the command line.

Now we realize that there's some problem and we have to remove it, so
we need to go through and remove this from our command line
everywhere.  Worse is that we have to proactively notice it and remove
it.

Instead, let's imagine that we set a policy at a bit of a higher
level.  Different ideas:

a) We could have a CONFIG_ option for the video codec that's something
like "CONFIG_VIDEOCODEC_DEFAULT_NONSTRICT".  If this was set then if
there is no "iommu.strict" command line argument then this device
would be mapped as non-strict.  If "iommu.strict=0" or
"iommu.strict=1" is on the command line then it would override all of
these defaults.  Probably the existence (or maybe the default value)
of this CONFIG option implies that there are no known/expected
exploits related to it.

jumping in a bit late so might have missed some of the background, but
shouldn't the driver decide strict vs non-strict?  It should be the
one that knows if non-strict is safe or not?

Part of the problem is that we need some way for the driver to know if
the connected device is external or not.  A soldered-down storage
device connected via an internal PCIe bus is probably OK to use w/
non-strict.  An exposed PCIe port that anyone could plug anything into
is probably not OK to use with non-strict.  Right now the kernel
doesn't have this info.

Part of the problem is also trying to describe the risk level that
you're OK with and/or which risks you're willing to take and which
ones you're not willing to take.  Right now there's a binary: "accept
all the risk so I can get the best performance" and "accept none of
the risk".  ...but what if you want to accept some risks and not
others?  Maybe you're compiling the firmware for the video decoder
yourself and you're very convinced that it really sanitizes all input
and can't be compromised (or, if it is, you can quickly patch the
compromise).  You might be willing to run this video codec in "non
strict mode".  ...but if you give the video decoder firmware to
someone else as a binary blob they might not be willing to trust you
and might want to run the same firmware as "non-strict".

BR,
-R

quoted

b) We could find some way to tag the video codec and then set
non-strictness on certain classes of devices, then we could have a
policy to disable strictness on certain classes of devices.

The nice thing about the above is that you could imagine someone
pushing a change to the stable trees that would fix everyone affected.
Nobody would need to go around and adjust command line options, they'd
just get the newest stable and it could cause devices to move into
strict mode if there was a known exploit.  I suppose with your
proposal stable trees could have a "blacklist" where the commandline
is ignored for exploited devices, but that seems ugly.

quoted

Device tree could be used but then we have a problem with it being for
only
describing hardware and it doesn't work for ACPI based systems.

Command line approach works for all systems (both DT and ACPI) without
having
to add too much churn to drivers. Lastly, I think we can have both
options, it
doesn't hurt to add command line parameter since it is optional.

I'm not opposed to something existing that lets you override this on
the command line, but I'm just not a fan of it being the primary way.

-Doug
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel