* Re: [PATCH 06/10] x86/cet: Add arch_prctl functions for shadow stack
From: Florian Weimer @ 2018-06-19 6:40 UTC (permalink / raw)
To: Kees Cook, Andy Lutomirski
Cc: H. J. Lu, Thomas Gleixner, Yu-cheng Yu, LKML, linux-doc, Linux-MM,
linux-arch, X86 ML, H. Peter Anvin, Ingo Molnar,
Shanbhogue, Vedvyas, Ravi V. Shankar, Dave Hansen,
Jonathan Corbet, Oleg Nesterov, Arnd Bergmann, mike.kravetz
In-Reply-To: <CAGXu5jK0gospOXRpN6zYiQPXOZeE=YpVAz2qu4Zc3-32v85+EQ@mail.gmail.com>
On 06/19/2018 02:52 AM, Kees Cook wrote:
> Adding Florian to CC, but if something gets CET enabled, it really
> shouldn't have a way to turn it off. If there's a way to turn it off,
> all the ROP research will suddenly turn to exactly one gadget before
> doing the rest of the ROP: turning off CET. Right now ROP is: use
> stack-pivot gadget, do everything else. Allowed CET to turn off will
> just add one step: use CET-off gadget, use stack-pivot gadget, do
> everything else. :P
>
> Following Linus's request for "slow introduction" of new security
> features, likely the best approach is to default to "relaxed" (with a
> warning about down-grades), and allow distros/end-users to pick
> "forced" if they know their libraries are all CET-enabled.
The dynamic linker can tell beforehand (before executing any user code)
whether a process image supports CET. So there doesn't have to be
anything gradual about it per se to preserve backwards compatibility.
The idea to turn off CET probably comes from the desire to support
dlopen. I'm not sure if this is really necessary because the complexity
is rather nasty. (We currently do something similar for executable
stacks.) I'd rather have a switch to turn off the feature upon process
start. Things like NSS and PAM modules need to be recompiled early. (I
hope that everything that goes directly to the network via custom
protocols or hardware such as smartcards is proxied via daemons these days.)
Thanks,
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 5/5] Documentation/ABI: Add details of PCI AER statistics
From: poza @ 2018-06-19 6:03 UTC (permalink / raw)
To: Rajat Jain
Cc: Bjorn Helgaas, Jonathan Corbet, Philippe Ombredanne, Kate Stewart,
Thomas Gleixner, Greg Kroah-Hartman, Frederick Lawler,
Busch, Keith, Gabriele Paoloni, Alexandru Gagniuc, Thomas Tai,
Steven Rostedt, linux-pci, linux-doc, Linux Kernel Mailing List,
Jes Sorensen, Kyle McMartin, Rajat Jain
In-Reply-To: <CACK8Z6EHDv0d5zwgKj2N+DyxKEj9dVkSoN2Y_HTOKv8L9p-OAg@mail.gmail.com>
On 2018-06-19 05:41, Rajat Jain wrote:
> Hello,
>
> On Sat, Jun 16, 2018 at 10:24 PM <poza@codeaurora.org> wrote:
>>
>> On 2018-05-23 23:28, Rajat Jain wrote:
>> > Add the PCI AER statistics details to
>> > Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>> > and provide a pointer to it in
>> > Documentation/PCI/pcieaer-howto.txt
>> >
>> > Signed-off-by: Rajat Jain <rajatja@google.com>
>> > ---
>> > v2: Move the documentation to Documentation/ABI/
>> >
>> > .../testing/sysfs-bus-pci-devices-aer_stats | 103 ++++++++++++++++++
>> > Documentation/PCI/pcieaer-howto.txt | 5 +
>> > 2 files changed, 108 insertions(+)
>> > create mode 100644
>> > Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>> >
>> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>> > b/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>> > new file mode 100644
>> > index 000000000000..f55c389290ac
>> > --- /dev/null
>> > +++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>> > @@ -0,0 +1,103 @@
>> > +==========================
>> > +PCIe Device AER statistics
>> > +==========================
>> > +These attributes show up under all the devices that are AER capable.
>> > These
>> > +statistical counters indicate the errors "as seen/reported by the
>> > device".
>> > +Note that this may mean that if an end point is causing problems, the
>> > AER
>> > +counters may increment at its link partner (e.g. root port) because
>> > the
>> > +errors will be "seen" / reported by the link partner and not the the
>> > +problematic end point itself (which may report all counters as 0 as it
>> > never
>> > +saw any problems).
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_total_cor_errs
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Total number of correctable errors seen and reported by
>> > this
>> > + PCI device using ERR_COR.
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_total_fatal_errs
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Total number of uncorrectable fatal errors seen and
>> > reported
>> > + by this PCI device using ERR_FATAL.
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_total_nonfatal_errs
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Total number of uncorrectable non-fatal errors seen and
>> > reported
>> > + by this PCI device using ERR_NONFATAL.
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_breakdown_correctable
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Breakdown of of correctable errors seen and reported by
>> > this
>> > + PCI device using ERR_COR. A sample result looks like this:
>> > +-----------------------------------------
>> > +Receiver Error = 0x174
>> > +Bad TLP = 0x19
>> > +Bad DLLP = 0x3
>> > +RELAY_NUM Rollover = 0x0
>> > +Replay Timer Timeout = 0x1
>> > +Advisory Non-Fatal = 0x0
>> > +Corrected Internal Error = 0x0
>> > +Header Log Overflow = 0x0
>> > +-----------------------------------------
>> why hex display ? decimal is easy to read as these are counters.
>
> Have no particular preference. Since these can be potentially large
> numbers, just had a random thought that hex might make it more
> concise. I can change to decimal if that is preferable.
>
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_breakdown_uncorrectable
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Breakdown of of correctable errors seen and reported by
>> > this
>> > + PCI device using ERR_FATAL or ERR_NONFATAL. A sample result
>> > + looks like this:
>> > +-----------------------------------------
>> > +Undefined = 0x0
>> > +Data Link Protocol = 0x0
>> > +Surprise Down Error = 0x0
>> > +Poisoned TLP = 0x0
>> > +Flow Control Protocol = 0x0
>> > +Completion Timeout = 0x0
>> > +Completer Abort = 0x0
>> > +Unexpected Completion = 0x0
>> > +Receiver Overflow = 0x0
>> > +Malformed TLP = 0x0
>> > +ECRC = 0x0
>> > +Unsupported Request = 0x0
>> > +ACS Violation = 0x0
>> > +Uncorrectable Internal Error = 0x0
>> > +MC Blocked TLP = 0x0
>> > +AtomicOp Egress Blocked = 0x0
>> > +TLP Prefix Blocked Error = 0x0
>> > +-----------------------------------------
>> > +
>> > +============================
>> > +PCIe Rootport AER statistics
>> > +============================
>> > +These attributes showup under only the rootports that are AER capable.
>> > These
>> > +indicate the number of error messages as "reported to" the rootport.
>> > Please note
>> > +that the rootports also transmit (internally) the ERR_* messages for
>> > errors seen
>> > +by the internal rootport PCI device, so these counters includes them
>> > and are
>> > +thus cumulative of all the error messages on the PCI hierarchy
>> > originating
>> > +at that root port.
>>
>> what about switches and bridges ?
>
> What about them? AIUI, the switches forward the ERR_ messages from
> downstream devices to the rootport, like they do with standard
> messages. They can potentially generate their own ERR_ message and
> that would be reported no different than other end point devices.
yes, what I meant to ask is; the ERR_FATAL msg coming from EP, can be
contained by switch
and the error handling code thinks that, the error is contained by
switch irrespective of
AER or DPC, and it will think that the problem could be with
Switch/bridge upstream link.
hence the pci_dev of the switch where you should be increment your
counters.
of course ER_FATAL would have traversed till RP, but that doesnt meant
that
you account the error there.
>
>> Also Can you give some idea as e.g what is the difference between
>> dev_total_fatal_errs and rootport_total_fatal_errs (assuming that
>> both
>> are same pci_dev.
>
> For a pci_dev representing the rootport:
>
> dev_total_fatal_errors = how many times this PCI device *experienced*
> a fatal problem on its own (i.e. either link issues while talking to
> its link partner, or some internal errors).
>
> rootport_total_fatal_errors = how many times this rootport was
> *informed* about a problem (via ERR_* messages) in the PCI hierarchy
> that originates at it (can be any link further downstream). This
> includes the dev_total_fatal_errors also, because any errors detected
> by the rootport are also "informed" to itself via ERR_* messages. In
> reality, this is just the total number of ERR_FATAL messages received
> at the rootport. This sysfs attribute will only exist for root ports.
>
>>
>> rootport_total_fatal_errs gives me an idea that how many times things
>> have been failed under this pci_dev ?
>
> Yes, as above.
>
>> which means num of downstream link problems. but I am still trying to
>> make sense as how it could be used,
>> since we dont have BDF information associated with the number of
>> errors
>> anywhere (except these AER print messages)
>>
>
> Agree. That is a limitation. The challenges being more record keeping,
> more complicated sysfs representation, and given that PCI devices may
> come and go, how do we know it is the same device before we collate
> their stats etc.
>
>>
>> and dev_total_fatal_errs as you mentioned above that problematic EP,
>> then say root-port will report it and increment
>> dev_total_fatal_errs ++
>> does it also increment root-port_total_fatal_errs ++ in above scenario
>> ?
>
> Yes, as above, it will also root-port_total_fatal_errs++ for the root
> port of that hierarchy.
>
> Thanks,
>
> Rajat
>
>>
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/rootport_total_cor_errs
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Total number of ERR_COR messages reported to rootport.
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/rootport_total_fatal_errs
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Total number of ERR_FATAL messages reported to rootport.
>> > +
>> > +Where:
>> > /sys/bus/pci/devices/<dev>/aer_stats/rootport_total_nonfatal_errs
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Total number of ERR_NONFATAL messages reported to
>> > rootport.
>> > diff --git a/Documentation/PCI/pcieaer-howto.txt
>> > b/Documentation/PCI/pcieaer-howto.txt
>> > index acd0dddd6bb8..91b6e677cb8c 100644
>> > --- a/Documentation/PCI/pcieaer-howto.txt
>> > +++ b/Documentation/PCI/pcieaer-howto.txt
>> > @@ -73,6 +73,11 @@ In the example, 'Requester ID' means the ID of the
>> > device who sends
>> > the error message to root port. Pls. refer to pci express specs for
>> > other fields.
>> >
>> > +2.4 AER Statistics / Counters
>> > +
>> > +When PCIe AER errors are captured, the counters / statistics are also
>> > exposed
>> > +in form of sysfs attributes which are documented at
>> > +Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>> >
>> > 3. Developer Guide
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] overlayfs: caller_credentials option bypass creator_cred
From: Amir Goldstein @ 2018-06-19 5:39 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Mark Salyzyn, linux-kernel, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, overlayfs, linux-doc
In-Reply-To: <87in6fagxw.fsf@xmission.com>
On Tue, Jun 19, 2018 at 1:05 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Mark Salyzyn <salyzyn@android.com> writes:
>
>> All accesses to the lower filesystems reference the creator (mount)
>> context. This is a security issue as the user context does not
>> overlay the creator context.
>
> Sigh. You gave Vivek a reasonable description of what is going on
> and then you did not repeat it here. Your patch description
> very much needs to be fixed.
>
> As I read your patch there you are removing an override in credentials.
> Presumably that override is needed for the fileystem to function.
>
> If that override is needed to function your patch is incorrect
> because it breaks things for no reason.
>
> If that override is not needed it should be safe to explain why
> and simply remove it from overlayfs.
>
Eric is correct about override being needed for filesystem to function
because overlayfs calls mknod and sets trusted xattr.
Alas, overlayfs makes an assumption that if mounter has credentials
to mount, it also has credentials for those other things, which so far,
nobody complained about AFAIK.
There was one complain though about abusing SYS_CAP_RESOURCE
of mounter to exceed underlying ext4 quotas, so that capability has been
recently revoked unconditionally from the override creds - that makes a user
with SYS_CAP_RESOURCE also incapable of exceeding underlying ext4
quotas (a good thing IMO, but that is arguable).
Ideally, the correct solution would be to "merge" the mounter creds and
caller creds and use different combinations of the two in different cases,
but that would complicate the code and the test matrix considerably.
> I don't see any real explanations in this change description.
> So this appears to be an incompletely thought out change.
>
>
> Mostly this looks like someone tried to use the principle of least
> privilege in your Android implementation and got it wrong. Having given
> the mounter of overlayfs too few privileges this appears to be an
> attempt to get overlayfs to pay the cost of an implementation mistake in
> the Android security model.
>
> That seems like a very unreasonable thing to do.
>
To the best of my knowledge, the Android sepolicy rules for init have been
like that for a very long time. I am not sure why Eric claims the unreasonable
claim. Not sure who the users of overlayfs are going to be though and if
it is possible to guaranty that they have capabilities to mknod and set trusted
xattr (needed for removing dirs among other things).
As it is, the patch is simple enough and only makes an existing functionality
optional, so if it *really* solves the Android use case, and there is
*really* no
other "reasonable" way to configure Android sepolicy, I rather have this patch
than the more "correct" implementation.
Two comments about the patch in case we are going forward with it:
1. I would use the same convention for Kconfig/module param/mount option
as used for other overlayfs defaults, i.e. override_creds=on/off and Kconfig
default which defaults to the legacy behavior.
2. with override_creds=off, all the checks being done during mount become
irrelevant (e.g. mounter can set trusted xattr), so either skip them and issue
a warning saying that we really hope mounter knows what it is doing, or
I don't know what.
Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2] Documentation/sphinx: add "nodocs" directive
From: Mike Rapoport @ 2018-06-19 5:03 UTC (permalink / raw)
To: Jani Nikula; +Cc: Jonathan Corbet, Matthew Wilcox, linux-doc
In-Reply-To: <87h8lzyicj.fsf@intel.com>
On Mon, Jun 18, 2018 at 11:01:32PM +0300, Jani Nikula wrote:
> On Mon, 18 Jun 2018, Mike Rapoport <rppt@linux.vnet.ibm.com> wrote:
> > When kernel-doc:: specified in .rst document without explicit directives,
> > it outputs both comment and DOC: sections. If a DOC: section was explictly
> > included in the same document it will be duplicated. For example, the
> > output generated for Documentation/core-api/idr.rst [1] has "IDA
> > description" in the "IDA usage" section and in the middle of the API
> > reference.
> >
> > Addition of "nodocs" directive prevents the duplication without the need to
> > explicitly define what functions should be include in the API reference.
> >
> > [1] https://www.kernel.org/doc/html/v4.17/core-api/idr.html
> >
> > Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
> > ---
> > Documentation/sphinx/kerneldoc.py | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
> > index fbedcc3..bc5dd05 100644
> > --- a/Documentation/sphinx/kerneldoc.py
> > +++ b/Documentation/sphinx/kerneldoc.py
> > @@ -50,6 +50,7 @@ class KernelDocDirective(Directive):
> > 'functions': directives.unchanged_required,
> > 'export': directives.unchanged,
> > 'internal': directives.unchanged,
> > + 'nodocs': directives.unchanged,
>
> I'm not convinved this is the prettiest way to achieve what you
> want. 'nodocs' seems kind of clunky.
>
> I'd suggest supporting 'functions' without option arguments, and turning
> that into kernel-doc -no-doc-sections.
Do you mean something like this:
diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
index fbedcc3..9d0a7f0 100644
--- a/Documentation/sphinx/kerneldoc.py
+++ b/Documentation/sphinx/kerneldoc.py
@@ -47,7 +47,7 @@ class KernelDocDirective(Directive):
optional_arguments = 4
option_spec = {
'doc': directives.unchanged_required,
- 'functions': directives.unchanged_required,
+ 'functions': directives.unchanged,
'export': directives.unchanged,
'internal': directives.unchanged,
}
@@ -75,8 +75,12 @@ class KernelDocDirective(Directive):
elif 'doc' in self.options:
cmd += ['-function', str(self.options.get('doc'))]
elif 'functions' in self.options:
- for f in str(self.options.get('functions')).split():
- cmd += ['-function', f]
+ functions = self.options.get('functions').split()
+ if functions:
+ for f in functions:
+ cmd += ['-function', f]
+ else:
+ cmd += ['-no-doc-sections']
for pattern in export_file_patterns:
for f in glob.glob(env.config.kerneldoc_srctree + '/' + pattern):
> The usage in patch 2/2 would turn into:
>
> .. kernel-doc:: include/linux/idr.h
> :functions:
>
> which I think is much better overall in the rst source, complementing
> the places where you use :doc:.
>
> BR,
> Jani.
>
> > }
> > has_content = False
> >
> > @@ -77,6 +78,8 @@ class KernelDocDirective(Directive):
> > elif 'functions' in self.options:
> > for f in str(self.options.get('functions')).split():
> > cmd += ['-function', f]
> > + elif 'nodocs' in self.options:
> > + cmd += ['-no-doc-sections']
> >
> > for pattern in export_file_patterns:
> > for f in glob.glob(env.config.kerneldoc_srctree + '/' + pattern):
>
> --
> Jani Nikula, Intel Open Source Graphics Center
>
--
Sincerely yours,
Mike.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v4 26/26] fix a series of Documentation/ broken file name references
From: Vinod @ 2018-06-19 4:14 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Jonathan Corbet, Mauro Carvalho Chehab,
linux-kernel, Harry Wei, Dan Williams, Darren Hart,
Andy Shevchenko, David S. Miller, Luis de Bethencourt,
Salah Triki, Mike Marshall, Martin Brandenburg, Rafael J. Wysocki,
Pavel Machek, Len Brown, linux-kernel, dmaengine,
platform-driver-x86, sparclinux, devel, linux-pm
In-Reply-To: <041e7a803fc0e5c5dfc1fb8b2f761074ca0baa2c.1529079120.git.mchehab+samsung@kernel.org>
On 15-06-18, 13:30, Mauro Carvalho Chehab wrote:
> If you have any comment or update to the content, please contact the
> original document maintainer directly. However, if you have a problem
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index b451354735d3..08ba8473a284 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -38,7 +38,7 @@
> * Each device has a channels list, which runs unlocked but is never modified
> * once the device is registered, it's just setup by the driver.
> *
> - * See Documentation/dmaengine.txt for more details
> + * See Documentation/driver-api/dmaengine for more details
DMAengine part:
Acked-by: Vinod Koul <vkoul@kernel.org>
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 06/10] x86/cet: Add arch_prctl functions for shadow stack
From: Kees Cook @ 2018-06-19 0:52 UTC (permalink / raw)
To: Andy Lutomirski
Cc: H. J. Lu, Thomas Gleixner, Yu-cheng Yu, LKML, linux-doc, Linux-MM,
linux-arch, X86 ML, H. Peter Anvin, Ingo Molnar,
Shanbhogue, Vedvyas, Ravi V. Shankar, Dave Hansen,
Jonathan Corbet, Oleg Nesterov, Arnd Bergmann, mike.kravetz,
Florian Weimer
In-Reply-To: <CALCETrVsh5t-V1Sm88LsZE_+DS0GE_bMWbcoX3SjD6GnrB08Pw@mail.gmail.com>
On Mon, Jun 18, 2018 at 3:03 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, Jun 12, 2018 at 12:34 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Tue, Jun 12, 2018 at 11:59 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > On Tue, 12 Jun 2018, H.J. Lu wrote:
>> >> On Tue, Jun 12, 2018 at 9:34 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> >> > On Tue, Jun 12, 2018 at 9:05 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>> >> >> On Tue, Jun 12, 2018 at 9:01 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> >> >> > On Tue, Jun 12, 2018 at 4:43 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>> >> >> >> On Tue, Jun 12, 2018 at 3:03 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> >> >> >> > That works for stuff which loads all libraries at start time, but what
>> >> >> >> > happens if the program uses dlopen() later on? If CET is force locked and
>> >> >> >> > the library is not CET enabled, it will fail.
>> >> >> >>
>> >> >> >> That is to prevent disabling CET by dlopening a legacy shared library.
>> >> >> >>
>> >> >> >> > I don't see the point of trying to support CET by magic. It adds complexity
>> >> >> >> > and you'll never be able to handle all corner cases correctly. dlopen() is
>> >> >> >> > not even a corner case.
>> >> >> >>
>> >> >> >> That is a price we pay for security. To enable CET, especially shadow
>> >> >> >> shack, the program and all of shared libraries it uses should be CET
>> >> >> >> enabled. Most of programs can be enabled with CET by compiling them
>> >> >> >> with -fcf-protection.
>> >> >> >
>> >> >> > If you charge too high a price for security, people may turn it off.
>> >> >> > I think we're going to need a mode where a program says "I want to use
>> >> >> > the CET, but turn it off if I dlopen an unsupported library". There
>> >> >> > are programs that load binary-only plugins.
>> >> >>
>> >> >> You can do
>> >> >>
>> >> >> # export GLIBC_TUNABLES=glibc.tune.hwcaps=-SHSTK
>> >> >>
>> >> >> which turns off shadow stack.
>> >> >>
>> >> >
>> >> > Which exactly illustrates my point. By making your security story too
>> >> > absolute, you'll force people to turn it off when they don't need to.
>> >> > If I'm using a fully CET-ified distro and I'm using a CET-aware
>> >> > program that loads binary plugins, and I may or may not have an old
>> >> > (binary-only, perhaps) plugin that doesn't support CET, then the
>> >> > behavior I want is for CET to be on until I dlopen() a program that
>> >> > doesn't support it. Unless there's some ABI reason why that can't be
>> >> > done, but I don't think there is.
>> >>
>> >> We can make it opt-in via GLIBC_TUNABLES. But by default, the legacy
>> >> shared object is disallowed when CET is enabled.
>> >
>> > That's a bad idea. Stuff has launchers which people might not be able to
>> > change. So they will simply turn of CET completely or it makes them hack
>> > horrible crap into init, e.g. the above export.
>> >
>> > Give them sane kernel options:
>> >
>> > cet = off, relaxed, forced
>> >
>> > where relaxed allows to run binary plugins. Then let dlopen() call into the
>> > kernel with the filepath of the library to check for CET and that will tell
>> > you whether its ok or or not and do the necessary magic in the kernel when
>> > CET has to be disabled due to a !CET library/application.
>> >
>> > That's also making the whole thing independent of magic glibc environment
>> > options and allows it to be used all over the place in the same way.
>>
>> This is very similar to our ARCH_CET_EXEC proposal which controls how
>> CET should be enforced. But Andy thinks it is a bad idea.
>
> I do think it's a bad idea to have a new piece of state that survives
> across exec(). It's going to have nasty usability problems and nasty
> security problems.
>
> We may need a mode by which glibc can turn CET *back off* even after a
> program had it on if it dlopens() an old binary. Or maybe there won't
> be demand. I can certainly understand why the CET_LOCK feature is
> there, although I think we need a way to override it using something
> like ptrace(). I'm not convinced that CET_LOCK is really needed, but
> someone who understand the thread model should chime in.
>
> Kees, do you know anyone who has a good enough understanding of
> usermode exploits and how they'll interact with CET?
Adding Florian to CC, but if something gets CET enabled, it really
shouldn't have a way to turn it off. If there's a way to turn it off,
all the ROP research will suddenly turn to exactly one gadget before
doing the rest of the ROP: turning off CET. Right now ROP is: use
stack-pivot gadget, do everything else. Allowed CET to turn off will
just add one step: use CET-off gadget, use stack-pivot gadget, do
everything else. :P
Following Linus's request for "slow introduction" of new security
features, likely the best approach is to default to "relaxed" (with a
warning about down-grades), and allow distros/end-users to pick
"forced" if they know their libraries are all CET-enabled.
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 5/5] Documentation/ABI: Add details of PCI AER statistics
From: Rajat Jain @ 2018-06-19 0:32 UTC (permalink / raw)
To: Rajat Jain
Cc: Oza Pawandeep, Bjorn Helgaas, Jonathan Corbet,
Philippe Ombredanne, Kate Stewart, Thomas Gleixner,
Greg Kroah-Hartman, Frederick Lawler, Busch, Keith,
Gabriele Paoloni, Alexandru Gagniuc, Thomas Tai, Steven Rostedt,
linux-pci, linux-doc, Linux Kernel Mailing List, Jes Sorensen,
Kyle McMartin
In-Reply-To: <CACK8Z6EHDv0d5zwgKj2N+DyxKEj9dVkSoN2Y_HTOKv8L9p-OAg@mail.gmail.com>
Sorry, correction needed in my statement below:
On Mon, Jun 18, 2018 at 5:11 PM, Rajat Jain <rajatja@google.com> wrote:
> Hello,
>
> On Sat, Jun 16, 2018 at 10:24 PM <poza@codeaurora.org> wrote:
>>
>> On 2018-05-23 23:28, Rajat Jain wrote:
>> > Add the PCI AER statistics details to
>> > Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>> > and provide a pointer to it in
>> > Documentation/PCI/pcieaer-howto.txt
>> >
>> > Signed-off-by: Rajat Jain <rajatja@google.com>
>> > ---
>> > v2: Move the documentation to Documentation/ABI/
>> >
>> > .../testing/sysfs-bus-pci-devices-aer_stats | 103 ++++++++++++++++++
>> > Documentation/PCI/pcieaer-howto.txt | 5 +
>> > 2 files changed, 108 insertions(+)
>> > create mode 100644
>> > Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>> >
>> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>> > b/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>> > new file mode 100644
>> > index 000000000000..f55c389290ac
>> > --- /dev/null
>> > +++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>> > @@ -0,0 +1,103 @@
>> > +==========================
>> > +PCIe Device AER statistics
>> > +==========================
>> > +These attributes show up under all the devices that are AER capable.
>> > These
>> > +statistical counters indicate the errors "as seen/reported by the
>> > device".
>> > +Note that this may mean that if an end point is causing problems, the
>> > AER
>> > +counters may increment at its link partner (e.g. root port) because
>> > the
>> > +errors will be "seen" / reported by the link partner and not the the
>> > +problematic end point itself (which may report all counters as 0 as it
>> > never
>> > +saw any problems).
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_total_cor_errs
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Total number of correctable errors seen and reported by
>> > this
>> > + PCI device using ERR_COR.
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_total_fatal_errs
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Total number of uncorrectable fatal errors seen and
>> > reported
>> > + by this PCI device using ERR_FATAL.
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_total_nonfatal_errs
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Total number of uncorrectable non-fatal errors seen and
>> > reported
>> > + by this PCI device using ERR_NONFATAL.
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_breakdown_correctable
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Breakdown of of correctable errors seen and reported by
>> > this
>> > + PCI device using ERR_COR. A sample result looks like this:
>> > +-----------------------------------------
>> > +Receiver Error = 0x174
>> > +Bad TLP = 0x19
>> > +Bad DLLP = 0x3
>> > +RELAY_NUM Rollover = 0x0
>> > +Replay Timer Timeout = 0x1
>> > +Advisory Non-Fatal = 0x0
>> > +Corrected Internal Error = 0x0
>> > +Header Log Overflow = 0x0
>> > +-----------------------------------------
>> why hex display ? decimal is easy to read as these are counters.
>
> Have no particular preference. Since these can be potentially large
> numbers, just had a random thought that hex might make it more
> concise. I can change to decimal if that is preferable.
>
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_breakdown_uncorrectable
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Breakdown of of correctable errors seen and reported by
>> > this
>> > + PCI device using ERR_FATAL or ERR_NONFATAL. A sample result
>> > + looks like this:
>> > +-----------------------------------------
>> > +Undefined = 0x0
>> > +Data Link Protocol = 0x0
>> > +Surprise Down Error = 0x0
>> > +Poisoned TLP = 0x0
>> > +Flow Control Protocol = 0x0
>> > +Completion Timeout = 0x0
>> > +Completer Abort = 0x0
>> > +Unexpected Completion = 0x0
>> > +Receiver Overflow = 0x0
>> > +Malformed TLP = 0x0
>> > +ECRC = 0x0
>> > +Unsupported Request = 0x0
>> > +ACS Violation = 0x0
>> > +Uncorrectable Internal Error = 0x0
>> > +MC Blocked TLP = 0x0
>> > +AtomicOp Egress Blocked = 0x0
>> > +TLP Prefix Blocked Error = 0x0
>> > +-----------------------------------------
>> > +
>> > +============================
>> > +PCIe Rootport AER statistics
>> > +============================
>> > +These attributes showup under only the rootports that are AER capable.
>> > These
>> > +indicate the number of error messages as "reported to" the rootport.
>> > Please note
>> > +that the rootports also transmit (internally) the ERR_* messages for
>> > errors seen
>> > +by the internal rootport PCI device, so these counters includes them
>> > and are
>> > +thus cumulative of all the error messages on the PCI hierarchy
>> > originating
>> > +at that root port.
>>
>> what about switches and bridges ?
>
> What about them? AIUI, the switches forward the ERR_ messages from
> downstream devices to the rootport, like they do with standard
> messages. They can potentially generate their own ERR_ message and
> that would be reported no different than other end point devices.
>
>> Also Can you give some idea as e.g what is the difference between
>> dev_total_fatal_errs and rootport_total_fatal_errs (assuming that both
>> are same pci_dev.
>
> For a pci_dev representing the rootport:
>
> dev_total_fatal_errors = how many times this PCI device *experienced*
> a fatal problem on its own (i.e. either link issues while talking to
> its link partner, or some internal errors).
>
> rootport_total_fatal_errors = how many times this rootport was
> *informed* about a problem (via ERR_* messages) in the PCI hierarchy
Read the above sentence as:
" rootport_total_fatal_errors = how many times this rootport was
*informed* about a FATAL problem (via ERR_FATAL messages) in the PCI hierarchy"
> that originates at it (can be any link further downstream). This
> includes the dev_total_fatal_errors also, because any errors detected
> by the rootport are also "informed" to itself via ERR_* messages. In
> reality, this is just the total number of ERR_FATAL messages received
> at the rootport. This sysfs attribute will only exist for root ports.
>
>>
>> rootport_total_fatal_errs gives me an idea that how many times things
>> have been failed under this pci_dev ?
>
> Yes, as above.
>
>> which means num of downstream link problems. but I am still trying to
>> make sense as how it could be used,
>> since we dont have BDF information associated with the number of errors
>> anywhere (except these AER print messages)
>>
>
> Agree. That is a limitation. The challenges being more record keeping,
> more complicated sysfs representation, and given that PCI devices may
> come and go, how do we know it is the same device before we collate
> their stats etc.
>
>>
>> and dev_total_fatal_errs as you mentioned above that problematic EP,
>> then say root-port will report it and increment
>> dev_total_fatal_errs ++
>> does it also increment root-port_total_fatal_errs ++ in above scenario ?
>
> Yes, as above, it will also root-port_total_fatal_errs++ for the root
> port of that hierarchy.
>
> Thanks,
>
> Rajat
>
>>
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/rootport_total_cor_errs
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Total number of ERR_COR messages reported to rootport.
>> > +
>> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/rootport_total_fatal_errs
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Total number of ERR_FATAL messages reported to rootport.
>> > +
>> > +Where:
>> > /sys/bus/pci/devices/<dev>/aer_stats/rootport_total_nonfatal_errs
>> > +Date: May 2018
>> > +Kernel Version: 4.17.0
>> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
>> > +Description: Total number of ERR_NONFATAL messages reported to
>> > rootport.
>> > diff --git a/Documentation/PCI/pcieaer-howto.txt
>> > b/Documentation/PCI/pcieaer-howto.txt
>> > index acd0dddd6bb8..91b6e677cb8c 100644
>> > --- a/Documentation/PCI/pcieaer-howto.txt
>> > +++ b/Documentation/PCI/pcieaer-howto.txt
>> > @@ -73,6 +73,11 @@ In the example, 'Requester ID' means the ID of the
>> > device who sends
>> > the error message to root port. Pls. refer to pci express specs for
>> > other fields.
>> >
>> > +2.4 AER Statistics / Counters
>> > +
>> > +When PCIe AER errors are captured, the counters / statistics are also
>> > exposed
>> > +in form of sysfs attributes which are documented at
>> > +Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>> >
>> > 3. Developer Guide
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 5/5] Documentation/ABI: Add details of PCI AER statistics
From: Rajat Jain @ 2018-06-19 0:11 UTC (permalink / raw)
To: Oza Pawandeep
Cc: Bjorn Helgaas, Jonathan Corbet, Philippe Ombredanne, Kate Stewart,
Thomas Gleixner, Greg Kroah-Hartman, Frederick Lawler,
Busch, Keith, Gabriele Paoloni, Alexandru Gagniuc, Thomas Tai,
Steven Rostedt, linux-pci, linux-doc, Linux Kernel Mailing List,
Jes Sorensen, Kyle McMartin, Rajat Jain
In-Reply-To: <c27290dd401bb2954db4959559c39a00@codeaurora.org>
Hello,
On Sat, Jun 16, 2018 at 10:24 PM <poza@codeaurora.org> wrote:
>
> On 2018-05-23 23:28, Rajat Jain wrote:
> > Add the PCI AER statistics details to
> > Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
> > and provide a pointer to it in
> > Documentation/PCI/pcieaer-howto.txt
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> > v2: Move the documentation to Documentation/ABI/
> >
> > .../testing/sysfs-bus-pci-devices-aer_stats | 103 ++++++++++++++++++
> > Documentation/PCI/pcieaer-howto.txt | 5 +
> > 2 files changed, 108 insertions(+)
> > create mode 100644
> > Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
> > b/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
> > new file mode 100644
> > index 000000000000..f55c389290ac
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
> > @@ -0,0 +1,103 @@
> > +==========================
> > +PCIe Device AER statistics
> > +==========================
> > +These attributes show up under all the devices that are AER capable.
> > These
> > +statistical counters indicate the errors "as seen/reported by the
> > device".
> > +Note that this may mean that if an end point is causing problems, the
> > AER
> > +counters may increment at its link partner (e.g. root port) because
> > the
> > +errors will be "seen" / reported by the link partner and not the the
> > +problematic end point itself (which may report all counters as 0 as it
> > never
> > +saw any problems).
> > +
> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_total_cor_errs
> > +Date: May 2018
> > +Kernel Version: 4.17.0
> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
> > +Description: Total number of correctable errors seen and reported by
> > this
> > + PCI device using ERR_COR.
> > +
> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_total_fatal_errs
> > +Date: May 2018
> > +Kernel Version: 4.17.0
> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
> > +Description: Total number of uncorrectable fatal errors seen and
> > reported
> > + by this PCI device using ERR_FATAL.
> > +
> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_total_nonfatal_errs
> > +Date: May 2018
> > +Kernel Version: 4.17.0
> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
> > +Description: Total number of uncorrectable non-fatal errors seen and
> > reported
> > + by this PCI device using ERR_NONFATAL.
> > +
> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_breakdown_correctable
> > +Date: May 2018
> > +Kernel Version: 4.17.0
> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
> > +Description: Breakdown of of correctable errors seen and reported by
> > this
> > + PCI device using ERR_COR. A sample result looks like this:
> > +-----------------------------------------
> > +Receiver Error = 0x174
> > +Bad TLP = 0x19
> > +Bad DLLP = 0x3
> > +RELAY_NUM Rollover = 0x0
> > +Replay Timer Timeout = 0x1
> > +Advisory Non-Fatal = 0x0
> > +Corrected Internal Error = 0x0
> > +Header Log Overflow = 0x0
> > +-----------------------------------------
> why hex display ? decimal is easy to read as these are counters.
Have no particular preference. Since these can be potentially large
numbers, just had a random thought that hex might make it more
concise. I can change to decimal if that is preferable.
> > +
> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/dev_breakdown_uncorrectable
> > +Date: May 2018
> > +Kernel Version: 4.17.0
> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
> > +Description: Breakdown of of correctable errors seen and reported by
> > this
> > + PCI device using ERR_FATAL or ERR_NONFATAL. A sample result
> > + looks like this:
> > +-----------------------------------------
> > +Undefined = 0x0
> > +Data Link Protocol = 0x0
> > +Surprise Down Error = 0x0
> > +Poisoned TLP = 0x0
> > +Flow Control Protocol = 0x0
> > +Completion Timeout = 0x0
> > +Completer Abort = 0x0
> > +Unexpected Completion = 0x0
> > +Receiver Overflow = 0x0
> > +Malformed TLP = 0x0
> > +ECRC = 0x0
> > +Unsupported Request = 0x0
> > +ACS Violation = 0x0
> > +Uncorrectable Internal Error = 0x0
> > +MC Blocked TLP = 0x0
> > +AtomicOp Egress Blocked = 0x0
> > +TLP Prefix Blocked Error = 0x0
> > +-----------------------------------------
> > +
> > +============================
> > +PCIe Rootport AER statistics
> > +============================
> > +These attributes showup under only the rootports that are AER capable.
> > These
> > +indicate the number of error messages as "reported to" the rootport.
> > Please note
> > +that the rootports also transmit (internally) the ERR_* messages for
> > errors seen
> > +by the internal rootport PCI device, so these counters includes them
> > and are
> > +thus cumulative of all the error messages on the PCI hierarchy
> > originating
> > +at that root port.
>
> what about switches and bridges ?
What about them? AIUI, the switches forward the ERR_ messages from
downstream devices to the rootport, like they do with standard
messages. They can potentially generate their own ERR_ message and
that would be reported no different than other end point devices.
> Also Can you give some idea as e.g what is the difference between
> dev_total_fatal_errs and rootport_total_fatal_errs (assuming that both
> are same pci_dev.
For a pci_dev representing the rootport:
dev_total_fatal_errors = how many times this PCI device *experienced*
a fatal problem on its own (i.e. either link issues while talking to
its link partner, or some internal errors).
rootport_total_fatal_errors = how many times this rootport was
*informed* about a problem (via ERR_* messages) in the PCI hierarchy
that originates at it (can be any link further downstream). This
includes the dev_total_fatal_errors also, because any errors detected
by the rootport are also "informed" to itself via ERR_* messages. In
reality, this is just the total number of ERR_FATAL messages received
at the rootport. This sysfs attribute will only exist for root ports.
>
> rootport_total_fatal_errs gives me an idea that how many times things
> have been failed under this pci_dev ?
Yes, as above.
> which means num of downstream link problems. but I am still trying to
> make sense as how it could be used,
> since we dont have BDF information associated with the number of errors
> anywhere (except these AER print messages)
>
Agree. That is a limitation. The challenges being more record keeping,
more complicated sysfs representation, and given that PCI devices may
come and go, how do we know it is the same device before we collate
their stats etc.
>
> and dev_total_fatal_errs as you mentioned above that problematic EP,
> then say root-port will report it and increment
> dev_total_fatal_errs ++
> does it also increment root-port_total_fatal_errs ++ in above scenario ?
Yes, as above, it will also root-port_total_fatal_errs++ for the root
port of that hierarchy.
Thanks,
Rajat
>
> > +
> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/rootport_total_cor_errs
> > +Date: May 2018
> > +Kernel Version: 4.17.0
> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
> > +Description: Total number of ERR_COR messages reported to rootport.
> > +
> > +Where: /sys/bus/pci/devices/<dev>/aer_stats/rootport_total_fatal_errs
> > +Date: May 2018
> > +Kernel Version: 4.17.0
> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
> > +Description: Total number of ERR_FATAL messages reported to rootport.
> > +
> > +Where:
> > /sys/bus/pci/devices/<dev>/aer_stats/rootport_total_nonfatal_errs
> > +Date: May 2018
> > +Kernel Version: 4.17.0
> > +Contact: linux-pci@vger.kernel.org, rajatja@google.com
> > +Description: Total number of ERR_NONFATAL messages reported to
> > rootport.
> > diff --git a/Documentation/PCI/pcieaer-howto.txt
> > b/Documentation/PCI/pcieaer-howto.txt
> > index acd0dddd6bb8..91b6e677cb8c 100644
> > --- a/Documentation/PCI/pcieaer-howto.txt
> > +++ b/Documentation/PCI/pcieaer-howto.txt
> > @@ -73,6 +73,11 @@ In the example, 'Requester ID' means the ID of the
> > device who sends
> > the error message to root port. Pls. refer to pci express specs for
> > other fields.
> >
> > +2.4 AER Statistics / Counters
> > +
> > +When PCIe AER errors are captured, the counters / statistics are also
> > exposed
> > +in form of sysfs attributes which are documented at
> > +Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
> >
> > 3. Developer Guide
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/3] PCI: Make specifying PCI devices in kernel parameters reusable
From: Logan Gunthorpe @ 2018-06-18 23:11 UTC (permalink / raw)
To: Andy Shevchenko, Alex Williamson
Cc: Linux Kernel Mailing List, linux-pci, Linux Documentation List,
Stephen Bates, Christoph Hellwig, Bjorn Helgaas, Jonathan Corbet,
Ingo Molnar, Thomas Gleixner, Paul E. McKenney, Marc Zyngier,
Kai-Heng Feng, Frederic Weisbecker, Dan Williams,
Jérôme Glisse, Benjamin Herrenschmidt,
Christian König
In-Reply-To: <CAHp75VefSEvpUvsdi=W6rEiOX01zaJeMtxQMDqmSthPJP3uJFA@mail.gmail.com>
On 18/06/18 05:06 PM, Andy Shevchenko wrote:
> On Tue, Jun 19, 2018 at 12:44 AM, Alex Williamson
>> There's probably really no path to resolve these, but acknowledging the
>> difference in this comment block might be helpful in the future.
>
> ...or introduce a parser part to allow user supply "any" instead of
> numeric value.
I think the main difficulty is maintaining backwards compatibility. If
anyone is already using zero as a parameter then we will break their system.
>>> + pr_info("PCI: Can't parse resource_alignment parameter: pci:%s\n",
>
>> The "pci:" prefix on %s doesn't make sense now, it was used above when
>> the pointer was already advanced past this token, now I believe it would
>> lead to "pci:pci:xxxx:yyyy" or "pci:xx:yy.z". Thanks,
>
> I'm just wondering if we can use pci_info() here, Or it makes no sense?
> Also, the original loglevel was an "error".
Yeah, I don't think pci_info() makes sense as it's not attached to a
specific device.
Not sure how I messed up the log level, but I'll fix it for v4.
Thanks,
Logan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/3] PCI: Make specifying PCI devices in kernel parameters reusable
From: Andy Shevchenko @ 2018-06-18 23:06 UTC (permalink / raw)
To: Alex Williamson
Cc: Logan Gunthorpe, Linux Kernel Mailing List, linux-pci,
Linux Documentation List, Stephen Bates, Christoph Hellwig,
Bjorn Helgaas, Jonathan Corbet, Ingo Molnar, Thomas Gleixner,
Paul E. McKenney, Marc Zyngier, Kai-Heng Feng,
Frederic Weisbecker, Dan Williams, Jérôme Glisse,
Benjamin Herrenschmidt, Christian König
In-Reply-To: <20180618154430.1af3d42f@t450s.home>
On Tue, Jun 19, 2018 at 12:44 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 18 Jun 2018 13:36:34 -0600
> Logan Gunthorpe <logang@deltatee.com> wrote:
> I realize this is not a change in behavior, but since we're spelling it
> out in a proper comment rather than burying it in the implementation,
> using 0 as a wildcard is rather questionable behavior. It always
> surprises me when I read this because pci_match_one_device() uses
> PCI_ANY_ID (~0) as a wildcard and as a result of struct pci_device_id
> using __u32 for these fields, we actually need to specify ffffffff on
> the commandline to get a wildcard match for dynamic ids. The latter is
> tedious to use, but I think it's more correct, and the use of a __u32 is
> probably attributed to the fact that 0xffff is only reserved for vendor
> ID, the spec doesn't seem to reserve any entries from the vendor's
> device ID range.
>
> There's probably really no path to resolve these, but acknowledging the
> difference in this comment block might be helpful in the future.
...or introduce a parser part to allow user supply "any" instead of
numeric value.
>> + pr_info("PCI: Can't parse resource_alignment parameter: pci:%s\n",
> The "pci:" prefix on %s doesn't make sense now, it was used above when
> the pointer was already advanced past this token, now I believe it would
> lead to "pci:pci:xxxx:yyyy" or "pci:xx:yy.z". Thanks,
I'm just wondering if we can use pci_info() here, Or it makes no sense?
Also, the original loglevel was an "error".
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 3/3] PCI: Introduce the disable_acs_redir parameter
From: Logan Gunthorpe @ 2018-06-18 22:25 UTC (permalink / raw)
To: Alex Williamson
Cc: linux-kernel, linux-pci, linux-doc, Stephen Bates,
Christoph Hellwig, Bjorn Helgaas, Jonathan Corbet, Ingo Molnar,
Thomas Gleixner, Paul E. McKenney, Marc Zyngier, Kai-Heng Feng,
Frederic Weisbecker, Dan Williams, Jérôme Glisse,
Benjamin Herrenschmidt, Christian König
In-Reply-To: <20180618162100.3619575e@t450s.home>
On 18/06/18 04:21 PM, Alex Williamson wrote:
> Seems that too much is taken from the dynamic resource alignment that
> doesn't necessarily apply to a read-only, commandline-only, device
> discovery-only option. I don't think we need locking, I don't think we
> need a massive static buffer, ideally perhaps even no buffer, just a
> pointer to commandline. Thanks,
Yeah, fair points. I'll take another look and see what I can strip out.
Thanks,
Logan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 3/3] PCI: Introduce the disable_acs_redir parameter
From: Alex Williamson @ 2018-06-18 22:21 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: linux-kernel, linux-pci, linux-doc, Stephen Bates,
Christoph Hellwig, Bjorn Helgaas, Jonathan Corbet, Ingo Molnar,
Thomas Gleixner, Paul E. McKenney, Marc Zyngier, Kai-Heng Feng,
Frederic Weisbecker, Dan Williams, Jérôme Glisse,
Benjamin Herrenschmidt, Christian König
In-Reply-To: <20180618193636.16210-4-logang@deltatee.com>
On Mon, 18 Jun 2018 13:36:36 -0600
Logan Gunthorpe <logang@deltatee.com> wrote:
> In order to support P2P traffic on a segment of the PCI hierarchy,
> we must be able to disable the ACS redirect bits for select
> PCI bridges. The bridges must be selected before the devices are
> discovered by the kernel and the IOMMU groups created. Therefore,
> a kernel command line parameter is created to specify devices
> which must have their ACS bits disabled.
>
> The new parameter takes a list of devices separated by a semicolon.
> Each device specified will have it's ACS redirect bits disabled.
> This is similar to the existing 'resource_alignment' parameter and just
> like it we also create a sysfs bus attribute which can be used to
> read the parameter. Writing the parameter is not supported
> as it would require forcibly hot plugging the affected device as
> well as all devices whose IOMMU groups might change.
Why do we need a sysfs attribute for this if it's static, can't we just
see it in /proc/cmdline? Seems to be a fair bit of overhead to support
for as little as we can do with it.
> The ACS Request P2P Request Redirect, P2P Completion Redirect and P2P
> Egress Control bits are disabled which is sufficient to always allow
> passing P2P traffic uninterrupted. The bits are set after the kernel
> (optionally) enables the ACS bits itself. It is also done regardless of
> whether the kernel sets the bits or not seeing some BIOS firmware is known
> to set the bits on boot.
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Stephen Bates <sbates@raithlin.com>
> Acked-by: Christian König <christian.koenig@amd.com>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 9 +++
> drivers/pci/pci.c | 103 +++++++++++++++++++++++-
> 2 files changed, 110 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index d45285e1ab6a..2ec36e258bb0 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3190,6 +3190,15 @@
> Adding the window is slightly risky (it may
> conflict with unreported devices), so this
> taints the kernel.
> + disable_acs_redir=<pci_dev>[; ...]
> + Specify one or more PCI devices (in the format
> + specified above) separated by semicolons.
> + Each device specified will have the PCI ACS
> + redirect capabilities forced off which will
> + allow P2P traffic between devices through
> + bridges without forcing it upstream. Note:
> + this removes isolation between devices and
> + will make the IOMMU groups less granular.
>
> pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
> Management.
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6fbad0492461..eb85bf507398 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2982,6 +2982,92 @@ void pci_request_acs(void)
> pci_acs_enable = 1;
> }
>
> +#define DISABLE_ACS_REDIR_PARAM_SIZE COMMAND_LINE_SIZE
> +static char disable_acs_redir_param[DISABLE_ACS_REDIR_PARAM_SIZE] = {0};
Hmm, wouldn't this be initialized to zero anyway? I'm surprised
resource alignment is already wasting this sort of space vs dynamically
allocating, I'm not sure it's a good example to follow.
> +static DEFINE_SPINLOCK(disable_acs_redir_lock);
> +
> +static ssize_t pci_set_disable_acs_redir_param(const char *buf, size_t count)
> +{
> + if (count > DISABLE_ACS_REDIR_PARAM_SIZE - 1)
> + count = DISABLE_ACS_REDIR_PARAM_SIZE - 1;
> + spin_lock(&disable_acs_redir_lock);
> + strncpy(disable_acs_redir_param, buf, count);
> + disable_acs_redir_param[count] = '\0';
> + spin_unlock(&disable_acs_redir_lock);
> + return count;
> +}
> +
> +static ssize_t pci_disable_acs_redir_show(struct bus_type *bus, char *buf)
> +{
> + size_t count;
> +
> + spin_lock(&disable_acs_redir_lock);
> + count = snprintf(buf, PAGE_SIZE, "%s\n", disable_acs_redir_param);
> + spin_unlock(&disable_acs_redir_lock);
> + return count;
> +}
> +
> +static BUS_ATTR(disable_acs_redir, 0444, pci_disable_acs_redir_show, NULL);
> +
> +static int __init pci_disable_acs_redir_sysfs_init(void)
> +{
> + return bus_create_file(&pci_bus_type, &bus_attr_disable_acs_redir);
> +}
> +late_initcall(pci_disable_acs_redir_sysfs_init);
> +
> +/**
> + * pci_disable_acs_redir - disable ACS redirect capabilities
> + * @dev: the PCI device
> + *
> + * For only devices specified in the disable_acs_redir parameter.
> + */
> +static void pci_disable_acs_redir(struct pci_dev *dev)
> +{
> + int ret = 0;
> + const char *p;
> + int pos;
> + u16 ctrl;
> +
> + spin_lock(&disable_acs_redir_lock);
> +
> + p = disable_acs_redir_param;
> + while (*p) {
> + ret = pci_dev_str_match(dev, p, &p);
> + if (ret < 0) {
> + pr_info_once("PCI: Can't parse disable_acs_redir parameter: %s\n",
> + disable_acs_redir_param);
> +
> + break;
> + } else if (ret == 1) {
> + /* Found a match */
> + break;
> + }
> +
> + if (*p != ';' && *p != ',') {
> + /* End of param or invalid format */
> + break;
> + }
> + p++;
> + }
> + spin_unlock(&disable_acs_redir_lock);
> +
> + if (ret != 1)
> + return;
> +
> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
> + if (!pos)
> + return;
> +
> + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> +
> + /* P2P Request & Completion Redirect */
> + ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC);
> +
> + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> +
> + pci_info(dev, "disabled ACS redirect\n");
> +}
Seems that too much is taken from the dynamic resource alignment that
doesn't necessarily apply to a read-only, commandline-only, device
discovery-only option. I don't think we need locking, I don't think we
need a massive static buffer, ideally perhaps even no buffer, just a
pointer to commandline. Thanks,
Alex
> +
> /**
> * pci_std_enable_acs - enable ACS on devices using standard ACS capabilites
> * @dev: the PCI device
> @@ -3021,12 +3107,22 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> void pci_enable_acs(struct pci_dev *dev)
> {
> if (!pci_acs_enable)
> - return;
> + goto disable_acs_redir;
>
> if (!pci_dev_specific_enable_acs(dev))
> - return;
> + goto disable_acs_redir;
>
> pci_std_enable_acs(dev);
> +
> +disable_acs_redir:
> + /*
> + * Note: pci_disable_acs_redir() must be called even if
> + * ACS is not enabled by the kernel because the firmware
> + * may have unexpectedly set the flags. So if we are told
> + * to disable it, we should always disable it after setting
> + * the kernel's default preferences.
> + */
> + pci_disable_acs_redir(dev);
> }
>
> static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags)
> @@ -5966,6 +6062,9 @@ static int __init pci_setup(char *str)
> pcie_bus_config = PCIE_BUS_PEER2PEER;
> } else if (!strncmp(str, "pcie_scan_all", 13)) {
> pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
> + } else if (!strncmp(str, "disable_acs_redir=", 18)) {
> + pci_set_disable_acs_redir_param(str + 18,
> + strlen(str + 18));
> } else {
> printk(KERN_ERR "PCI: Unknown option `%s'\n",
> str);
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] overlayfs: caller_credentials option bypass creator_cred
From: Eric W. Biederman @ 2018-06-18 22:05 UTC (permalink / raw)
To: Mark Salyzyn
Cc: linux-kernel, Miklos Szeredi, Jonathan Corbet, Vivek Goyal,
linux-unionfs, linux-doc
In-Reply-To: <20180618192726.67981-1-salyzyn@android.com>
Mark Salyzyn <salyzyn@android.com> writes:
> All accesses to the lower filesystems reference the creator (mount)
> context. This is a security issue as the user context does not
> overlay the creator context.
Sigh. You gave Vivek a reasonable description of what is going on
and then you did not repeat it here. Your patch description
very much needs to be fixed.
As I read your patch there you are removing an override in credentials.
Presumably that override is needed for the fileystem to function.
If that override is needed to function your patch is incorrect
because it breaks things for no reason.
If that override is not needed it should be safe to explain why
and simply remove it from overlayfs.
I don't see any real explanations in this change description.
So this appears to be an incompletely thought out change.
Mostly this looks like someone tried to use the principle of least
privilege in your Android implementation and got it wrong. Having given
the mounter of overlayfs too few privileges this appears to be an
attempt to get overlayfs to pay the cost of an implementation mistake in
the Android security model.
That seems like a very unreasonable thing to do.
> A module bool parameter and mount option caller_credentials is added
> to set the default, and to act as a presence check for this "feature".
> The module parameter is used to change the default behavior, But
> since I can not break user API, I have to preserve the original
> behavior as default.
Changes to the user API are allowed if no one cares. Who cares?
Why is your change viable at all?
> On MAC security model expect to set the caller_credentials to Y as
> part of early initialization to preserve security model when the
> option of iether caller_credentials or creator_credentials is not
> explicitly specified in the mount.
Not an any random MAC security model. On a strange Android MAC security
model.
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: linux-unionfs@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
>
> v2:
> - forward port changed attr to stat, resulting in a build error
> - wording in commit message was too adversarial and needed correction
> ---
> Documentation/filesystems/overlayfs.txt | 7 +++++++
> fs/overlayfs/copy_up.c | 3 ++-
> fs/overlayfs/dir.c | 12 ++++++++----
> fs/overlayfs/inode.c | 24 ++++++++++++++++--------
> fs/overlayfs/namei.c | 9 ++++++---
> fs/overlayfs/ovl_entry.h | 1 +
> fs/overlayfs/readdir.c | 6 ++++--
> fs/overlayfs/super.c | 22 ++++++++++++++++++++++
> fs/overlayfs/util.c | 8 ++++++--
> 9 files changed, 72 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> index 72615a2c0752..4328be5cdaa9 100644
> --- a/Documentation/filesystems/overlayfs.txt
> +++ b/Documentation/filesystems/overlayfs.txt
> @@ -106,6 +106,13 @@ Only the lists of names from directories are merged. Other content
> such as metadata and extended attributes are reported for the upper
> directory only. These attributes of the lower directory are hidden.
>
> +credentials
> +-----------
> +
> +All access to the upper, lower and work directories is the creator's
> +credentials. If caller_credentials is set, then the access caller's
> +instance credentials will be used.
> +
> whiteouts and opaque directories
> --------------------------------
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index ddaddb4ce4c3..abc21844f712 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -790,7 +790,8 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
> dput(parent);
> dput(next);
> }
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
>
> return err;
> }
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index f480b1a2cd2e..8b75e0899ee1 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -561,7 +561,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> override_cred->fsgid = inode->i_gid;
> if (!attr->hardlink) {
> err = security_dentry_create_files_as(dentry,
> - attr->mode, &dentry->d_name, old_cred,
> + attr->mode, &dentry->d_name,
> + old_cred ? old_cred : current_cred(),
> override_cred);
> if (err) {
> put_cred(override_cred);
> @@ -577,7 +578,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> err = ovl_create_over_whiteout(dentry, inode, attr);
> }
> out_revert_creds:
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
> return err;
> }
>
> @@ -824,7 +826,8 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
> err = ovl_remove_upper(dentry, is_dir, &list);
> else
> err = ovl_remove_and_whiteout(dentry, &list);
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
> if (!err) {
> if (is_dir)
> clear_nlink(dentry->d_inode);
> @@ -1150,7 +1153,8 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
> out_unlock:
> unlock_rename(new_upperdir, old_upperdir);
> out_revert_creds:
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
> ovl_nlink_end(new, locked);
> out_drop_write:
> ovl_drop_write(old);
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index ed16a898caeb..222678b6e67e 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -49,7 +49,8 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
> inode_lock(upperdentry->d_inode);
> old_cred = ovl_override_creds(dentry->d_sb);
> err = notify_change(upperdentry, attr, NULL);
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
> if (!err)
> ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
> inode_unlock(upperdentry->d_inode);
> @@ -208,7 +209,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> stat->nlink = dentry->d_inode->i_nlink;
>
> out:
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
>
> return err;
> }
> @@ -242,7 +244,8 @@ int ovl_permission(struct inode *inode, int mask)
> mask |= MAY_READ;
> }
> err = inode_permission(realinode, mask);
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
>
> return err;
> }
> @@ -259,7 +262,8 @@ static const char *ovl_get_link(struct dentry *dentry,
>
> old_cred = ovl_override_creds(dentry->d_sb);
> p = vfs_get_link(ovl_dentry_real(dentry), done);
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
> return p;
> }
>
> @@ -302,7 +306,8 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
> WARN_ON(flags != XATTR_REPLACE);
> err = vfs_removexattr(realdentry, name);
> }
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
>
> out_drop_write:
> ovl_drop_write(dentry);
> @@ -320,7 +325,8 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
>
> old_cred = ovl_override_creds(dentry->d_sb);
> res = vfs_getxattr(realdentry, name, value, size);
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
> return res;
> }
>
> @@ -344,7 +350,8 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
>
> old_cred = ovl_override_creds(dentry->d_sb);
> res = vfs_listxattr(realdentry, list, size);
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
> if (res <= 0 || size == 0)
> return res;
>
> @@ -379,7 +386,8 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
>
> old_cred = ovl_override_creds(inode->i_sb);
> acl = get_acl(realinode, type);
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
>
> return acl;
> }
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index c993dd8db739..c8b84d262ec2 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1024,7 +1024,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> OVL_I(inode)->redirect = upperredirect;
> }
>
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
> dput(index);
> kfree(stack);
> kfree(d.redirect);
> @@ -1043,7 +1044,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> kfree(upperredirect);
> out:
> kfree(d.redirect);
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
> return ERR_PTR(err);
> }
>
> @@ -1097,7 +1099,8 @@ bool ovl_lower_positive(struct dentry *dentry)
> dput(this);
> }
> }
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
>
> return positive;
> }
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 41655a7d6894..7e17db561a04 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -18,6 +18,7 @@ struct ovl_config {
> const char *redirect_mode;
> bool index;
> bool nfs_export;
> + bool caller_credentials;
> int xino;
> };
>
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index ef1fe42ff7bb..af3874d589ad 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -289,7 +289,8 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
> }
> inode_unlock(dir->d_inode);
> }
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
>
> return err;
> }
> @@ -906,7 +907,8 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
>
> old_cred = ovl_override_creds(dentry->d_sb);
> err = ovl_dir_read_merged(dentry, list, &root);
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
> if (err)
> return err;
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 704b37311467..184688ab35cb 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -56,6 +56,11 @@ module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644);
> MODULE_PARM_DESC(ovl_xino_auto_def,
> "Auto enable xino feature");
>
> +static bool __read_mostly ovl_caller_credentials;
> +module_param_named(caller_credentials, ovl_caller_credentials, bool, 0644);
> +MODULE_PARM_DESC(ovl_caller_credentials,
> + "Use caller credentials rather than creator credentials for accesses");
> +
> static void ovl_entry_stack_free(struct ovl_entry *oe)
> {
> unsigned int i;
> @@ -376,6 +381,10 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
> "on" : "off");
> if (ofs->config.xino != ovl_xino_def())
> seq_printf(m, ",xino=%s", ovl_xino_str[ofs->config.xino]);
> + if (ofs->config.caller_credentials)
> + seq_puts(m, ",caller_credentials");
> + else
> + seq_puts(m, ",creator_credentials");
> return 0;
> }
>
> @@ -413,6 +422,8 @@ enum {
> OPT_XINO_ON,
> OPT_XINO_OFF,
> OPT_XINO_AUTO,
> + OPT_CREATOR_CREDENTIALS,
> + OPT_CALLER_CREDENTIALS,
> OPT_ERR,
> };
>
> @@ -429,6 +440,8 @@ static const match_table_t ovl_tokens = {
> {OPT_XINO_ON, "xino=on"},
> {OPT_XINO_OFF, "xino=off"},
> {OPT_XINO_AUTO, "xino=auto"},
> + {OPT_CREATOR_CREDENTIALS, "creator_credentials"},
> + {OPT_CALLER_CREDENTIALS, "caller_credentials"},
> {OPT_ERR, NULL}
> };
>
> @@ -486,6 +499,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> if (!config->redirect_mode)
> return -ENOMEM;
>
> + config->caller_credentials = ovl_caller_credentials;
> while ((p = ovl_next_opt(&opt)) != NULL) {
> int token;
> substring_t args[MAX_OPT_ARGS];
> @@ -555,6 +569,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> config->xino = OVL_XINO_AUTO;
> break;
>
> + case OPT_CREATOR_CREDENTIALS:
> + config->caller_credentials = false;
> + break;
> +
> + case OPT_CALLER_CREDENTIALS:
> + config->caller_credentials = true;
> + break;
> +
> default:
> pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
> return -EINVAL;
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 6f1078028c66..538802289511 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -40,6 +40,8 @@ const struct cred *ovl_override_creds(struct super_block *sb)
> {
> struct ovl_fs *ofs = sb->s_fs_info;
>
> + if (ofs->config.caller_credentials)
> + return NULL;
> return override_creds(ofs->creator_cred);
> }
>
> @@ -630,7 +632,8 @@ int ovl_nlink_start(struct dentry *dentry, bool *locked)
> * value relative to the upper inode nlink in an upper inode xattr.
> */
> err = ovl_set_nlink_upper(dentry);
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
>
> out:
> if (err)
> @@ -650,7 +653,8 @@ void ovl_nlink_end(struct dentry *dentry, bool locked)
>
> old_cred = ovl_override_creds(dentry->d_sb);
> ovl_cleanup_index(dentry);
> - revert_creds(old_cred);
> + if (old_cred)
> + revert_creds(old_cred);
> }
>
> mutex_unlock(&OVL_I(d_inode(dentry))->lock);
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 06/10] x86/cet: Add arch_prctl functions for shadow stack
From: Andy Lutomirski @ 2018-06-18 22:03 UTC (permalink / raw)
To: H. J. Lu, Kees Cook
Cc: Thomas Gleixner, Andrew Lutomirski, Yu-cheng Yu, LKML, linux-doc,
Linux-MM, linux-arch, X86 ML, H. Peter Anvin, Ingo Molnar,
Shanbhogue, Vedvyas, Ravi V. Shankar, Dave Hansen,
Jonathan Corbet, Oleg Nesterov, Arnd Bergmann, mike.kravetz
In-Reply-To: <CAMe9rOrGjJf0aMnUjAP38MqvOiW3=iXGQjcUT3O=f9pE85hXaw@mail.gmail.com>
On Tue, Jun 12, 2018 at 12:34 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jun 12, 2018 at 11:59 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Tue, 12 Jun 2018, H.J. Lu wrote:
> >> On Tue, Jun 12, 2018 at 9:34 AM, Andy Lutomirski <luto@kernel.org> wrote:
> >> > On Tue, Jun 12, 2018 at 9:05 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >> >> On Tue, Jun 12, 2018 at 9:01 AM, Andy Lutomirski <luto@kernel.org> wrote:
> >> >> > On Tue, Jun 12, 2018 at 4:43 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >> >> >> On Tue, Jun 12, 2018 at 3:03 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> >> >> > That works for stuff which loads all libraries at start time, but what
> >> >> >> > happens if the program uses dlopen() later on? If CET is force locked and
> >> >> >> > the library is not CET enabled, it will fail.
> >> >> >>
> >> >> >> That is to prevent disabling CET by dlopening a legacy shared library.
> >> >> >>
> >> >> >> > I don't see the point of trying to support CET by magic. It adds complexity
> >> >> >> > and you'll never be able to handle all corner cases correctly. dlopen() is
> >> >> >> > not even a corner case.
> >> >> >>
> >> >> >> That is a price we pay for security. To enable CET, especially shadow
> >> >> >> shack, the program and all of shared libraries it uses should be CET
> >> >> >> enabled. Most of programs can be enabled with CET by compiling them
> >> >> >> with -fcf-protection.
> >> >> >
> >> >> > If you charge too high a price for security, people may turn it off.
> >> >> > I think we're going to need a mode where a program says "I want to use
> >> >> > the CET, but turn it off if I dlopen an unsupported library". There
> >> >> > are programs that load binary-only plugins.
> >> >>
> >> >> You can do
> >> >>
> >> >> # export GLIBC_TUNABLES=glibc.tune.hwcaps=-SHSTK
> >> >>
> >> >> which turns off shadow stack.
> >> >>
> >> >
> >> > Which exactly illustrates my point. By making your security story too
> >> > absolute, you'll force people to turn it off when they don't need to.
> >> > If I'm using a fully CET-ified distro and I'm using a CET-aware
> >> > program that loads binary plugins, and I may or may not have an old
> >> > (binary-only, perhaps) plugin that doesn't support CET, then the
> >> > behavior I want is for CET to be on until I dlopen() a program that
> >> > doesn't support it. Unless there's some ABI reason why that can't be
> >> > done, but I don't think there is.
> >>
> >> We can make it opt-in via GLIBC_TUNABLES. But by default, the legacy
> >> shared object is disallowed when CET is enabled.
> >
> > That's a bad idea. Stuff has launchers which people might not be able to
> > change. So they will simply turn of CET completely or it makes them hack
> > horrible crap into init, e.g. the above export.
> >
> > Give them sane kernel options:
> >
> > cet = off, relaxed, forced
> >
> > where relaxed allows to run binary plugins. Then let dlopen() call into the
> > kernel with the filepath of the library to check for CET and that will tell
> > you whether its ok or or not and do the necessary magic in the kernel when
> > CET has to be disabled due to a !CET library/application.
> >
> > That's also making the whole thing independent of magic glibc environment
> > options and allows it to be used all over the place in the same way.
>
> This is very similar to our ARCH_CET_EXEC proposal which controls how
> CET should be enforced. But Andy thinks it is a bad idea.
>
I do think it's a bad idea to have a new piece of state that survives
across exec(). It's going to have nasty usability problems and nasty
security problems.
We may need a mode by which glibc can turn CET *back off* even after a
program had it on if it dlopens() an old binary. Or maybe there won't
be demand. I can certainly understand why the CET_LOCK feature is
there, although I think we need a way to override it using something
like ptrace(). I'm not convinced that CET_LOCK is really needed, but
someone who understand the thread model should chime in.
Kees, do you know anyone who has a good enough understanding of
usermode exploits and how they'll interact with CET?
--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: overlayfs: caller_credentials option bypass creator_cred
From: Mark Salyzyn @ 2018-06-18 21:59 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-kernel, Miklos Szeredi, Jonathan Corbet, linux-unionfs,
linux-doc, Daniel Walsh, Stephen Smalley
In-Reply-To: <20180618194345.GA15973@redhat.com>
On 06/18/2018 12:43 PM, Vivek Goyal wrote:
> Will it be acceptable to write security policies in such a way so that
> mounter has access as well.
Unfortunately No. Policy of minimizing attack surface for a contained
root service (init in this case). Just because it can mount, does not
mean it can modify critical content; an attacker could use this to open
a hole.
> Current model does assume that mounter has privileges on underlying files.
Only ones it appears to need is the workdir AFAIK, had to add ability to
create in the <wordir> xattr in order to enable r/w mounts later.
Although not all corners were tested, I did not see any copy_up issues
b/c the caller had the privs in the Android security model when mounted
with this new flag.
-- Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/3] PCI: Allow specifying devices using a base bus and path of devfns
From: Alex Williamson @ 2018-06-18 21:55 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: linux-kernel, linux-pci, linux-doc, Stephen Bates,
Christoph Hellwig, Bjorn Helgaas, Jonathan Corbet, Ingo Molnar,
Thomas Gleixner, Paul E. McKenney, Marc Zyngier, Kai-Heng Feng,
Frederic Weisbecker, Dan Williams, Jérôme Glisse,
Benjamin Herrenschmidt, Christian König
In-Reply-To: <20180618193636.16210-3-logang@deltatee.com>
On Mon, 18 Jun 2018 13:36:35 -0600
Logan Gunthorpe <logang@deltatee.com> wrote:
> When specifying PCI devices on the kernel command line using a
> BDF, the bus numbers can change when adding or replacing a device,
> changing motherboard firmware, or applying kernel parameters like
> pci=assign-buses. When this happens, it is usually undesirable to
> apply whatever command line tweak to the wrong device.
>
> Therefore, it is useful to be able to specify devices with a base
> bus number and the path of devfns needed to get to it. (Similar to
> the "device scope" structure in the Intel VT-d spec, Section 8.3.1.)
>
> Thus, we add an option to specify devices in the following format:
>
> [<domain>:]<bus>:<slot>.<func>[/<slot>.<func>][/ ...]
>
> The path can be any segment within the PCI hierarchy of any length and
> determined through the use of 'lspci -t'. When specified this way, it is
> less likely that a renumbered bus will result in a valid device specification
> and the tweak won't be applied to the wrong device.
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Stephen Bates <sbates@raithlin.com>
> Acked-by: Christian König <christian.koenig@amd.com>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 8 +-
> drivers/pci/pci.c | 120 ++++++++++++++++++++----
> 2 files changed, 106 insertions(+), 22 deletions(-)
Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/3] PCI: Make specifying PCI devices in kernel parameters reusable
From: Logan Gunthorpe @ 2018-06-18 21:49 UTC (permalink / raw)
To: Alex Williamson
Cc: linux-kernel, linux-pci, linux-doc, Stephen Bates,
Christoph Hellwig, Bjorn Helgaas, Jonathan Corbet, Ingo Molnar,
Thomas Gleixner, Paul E. McKenney, Marc Zyngier, Kai-Heng Feng,
Frederic Weisbecker, Dan Williams, Jérôme Glisse,
Benjamin Herrenschmidt, Christian König
In-Reply-To: <20180618154430.1af3d42f@t450s.home>
On 6/18/2018 3:44 PM, Alex Williamson wrote:
>> + *
>> + * The second format matches devices using IDs in the configuration
>> + * space which may match multiple devices in the system. A value of 0
>> + * for any field will match all devices.
>
> I realize this is not a change in behavior, but since we're spelling it
> out in a proper comment rather than burying it in the implementation,
> using 0 as a wildcard is rather questionable behavior. It always
> surprises me when I read this because pci_match_one_device() uses
> PCI_ANY_ID (~0) as a wildcard and as a result of struct pci_device_id
> using __u32 for these fields, we actually need to specify ffffffff on
> the commandline to get a wildcard match for dynamic ids. The latter is
> tedious to use, but I think it's more correct, and the use of a __u32 is
> probably attributed to the fact that 0xffff is only reserved for vendor
> ID, the spec doesn't seem to reserve any entries from the vendor's
> device ID range.
>
> There's probably really no path to resolve these, but acknowledging the
> difference in this comment block might be helpful in the future.
Ok, I'll add a note in the comment.
>> + ret = pci_dev_str_match(dev, p, &p);
>> + if (ret == 1) {
>> + *resize = true;
>> + if (align_order == -1)
>> + align = PAGE_SIZE;
>> + else
>> + align = 1 << align_order;
>> + break;
>> + } else if (ret < 0) {
>> + pr_info("PCI: Can't parse resource_alignment parameter: pci:%s\n",
>
>
> The "pci:" prefix on %s doesn't make sense now, it was used above when
> the pointer was already advanced past this token, now I believe it would
> lead to "pci:pci:xxxx:yyyy" or "pci:xx:yy.z". Thanks,
Yup, nice catch. I'll fix it for v4.
Logan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 00/10] Control Flow Enforcement - Part (3)
From: Andy Lutomirski @ 2018-06-18 21:44 UTC (permalink / raw)
To: Balbir Singh
Cc: Yu-cheng Yu, LKML, linux-doc, Linux-MM, linux-arch, X86 ML,
H. Peter Anvin, Thomas Gleixner, Ingo Molnar, H. J. Lu,
Shanbhogue, Vedvyas, Ravi V. Shankar, Dave Hansen,
Jonathan Corbet, Oleg Nesterov, Arnd Bergmann, mike.kravetz
In-Reply-To: <2b77abb17dfaf58b7c23fac9d8603482e1887337.camel@gmail.com>
On Sat, Jun 16, 2018 at 8:16 PM Balbir Singh <bsingharora@gmail.com> wrote:
>
> On Thu, 2018-06-14 at 07:56 -0700, Yu-cheng Yu wrote:
> > On Thu, 2018-06-14 at 11:07 +1000, Balbir Singh wrote:
> > > On Tue, 2018-06-12 at 08:03 -0700, Yu-cheng Yu wrote:
> > > > On Tue, 2018-06-12 at 20:56 +1000, Balbir Singh wrote:
> > > > >
> > > > > On 08/06/18 00:37, Yu-cheng Yu wrote:
> > > > > > This series introduces CET - Shadow stack
> > > > > >
> > > > > > At the high level, shadow stack is:
> > > > > >
> > > > > > Allocated from a task's address space with vm_flags VM_SHSTK;
> > > > > > Its PTEs must be read-only and dirty;
> > > > > > Fixed sized, but the default size can be changed by sys admin.
> > > > > >
> > > > > > For a forked child, the shadow stack is duplicated when the next
> > > > > > shadow stack access takes place.
> > > > > >
> > > > > > For a pthread child, a new shadow stack is allocated.
> > > > > >
> > > > > > The signal handler uses the same shadow stack as the main program.
> > > > > >
> > > > >
> > > > > Even with sigaltstack()?
> > > > >
> > > >
> > > > Yes.
> > >
> > > I am not convinced that it would work, as we switch stacks, oveflow might
> > > be an issue. I also forgot to bring up setcontext(2), I presume those
> > > will get new shadow stacks
> >
> > Do you mean signal stack/sigaltstack overflow or swapcontext in a signal
> > handler?
> >
>
> I meant any combination of that. If there is a user space threads implementation that uses sigaltstack for switching threads
>
Anyone who does that is nuts. The whole point of user space threads
is speed, and signals are very slow. For userspace threads to work,
we need an API to allocate new shadow stacks, and we need to use the
extremely awkwardly defined RSTORSSP stuff to switch. (I assume this
is possible on an ISA level. The docs are bad, and the mnemonics for
the relevant instructions are nonsensical.)
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/3] PCI: Make specifying PCI devices in kernel parameters reusable
From: Alex Williamson @ 2018-06-18 21:44 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: linux-kernel, linux-pci, linux-doc, Stephen Bates,
Christoph Hellwig, Bjorn Helgaas, Jonathan Corbet, Ingo Molnar,
Thomas Gleixner, Paul E. McKenney, Marc Zyngier, Kai-Heng Feng,
Frederic Weisbecker, Dan Williams, Jérôme Glisse,
Benjamin Herrenschmidt, Christian König
In-Reply-To: <20180618193636.16210-2-logang@deltatee.com>
On Mon, 18 Jun 2018 13:36:34 -0600
Logan Gunthorpe <logang@deltatee.com> wrote:
> Separate out the code to match a PCI device with a string (typically
> originating from a kernel parameter) from the
> pci_specified_resource_alignment() function into its own helper
> function.
>
> While we are at it, this change fixes the kernel style of the function
> (fixing a number of long lines and extra parentheses).
>
> Additionally, make the analogous change to the kernel parameter
> documentation: Separating the description of how to specify a PCI device
> into it's own section at the head of the pci= parameter.
>
> This patch should have no functional alterations.
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Stephen Bates <sbates@raithlin.com>
> Acked-by: Christian König <christian.koenig@amd.com>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 26 +++-
> drivers/pci/pci.c | 153 +++++++++++++++---------
> 2 files changed, 120 insertions(+), 59 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index efc7aa7a0670..760fb2b0b349 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2994,7 +2994,24 @@
> See header of drivers/block/paride/pcd.c.
> See also Documentation/blockdev/paride.txt.
>
> - pci=option[,option...] [PCI] various PCI subsystem options:
> + pci=option[,option...] [PCI] various PCI subsystem options.
> +
> + Some options herein operate on a specific device
> + or a set of devices (<pci_dev>). These are
> + specified in one of the following formats:
> +
> + [<domain>:]<bus>:<slot>.<func>
> + pci:<vendor>:<device>[:<subvendor>:<subdevice>]
> +
> + Note: the first format specifies a PCI
> + bus/slot/function address which may change
> + if new hardware is inserted, if motherboard
> + firmware changes, or due to changes caused
> + by other kernel parameters. The second format
Nit, is it worth noting that when unspecified, domain is zero?
> + selects devices using IDs from the
> + configuration space which may match multiple
> + devices in the system.
> +
> earlydump [X86] dump PCI config space before the kernel
> changes anything
> off [X86] don't probe for the PCI bus
> @@ -3123,11 +3140,10 @@
> window. The default value is 64 megabytes.
> resource_alignment=
> Format:
> - [<order of align>@][<domain>:]<bus>:<slot>.<func>[; ...]
> - [<order of align>@]pci:<vendor>:<device>\
> - [:<subvendor>:<subdevice>][; ...]
> + [<order of align>@]<pci_dev>[; ...]
> Specifies alignment and device to reassign
> - aligned memory resources.
> + aligned memory resources. How to
> + specify the device is described above.
> If <order of align> is not specified,
> PAGE_SIZE is used as alignment.
> PCI-PCI bridge can be specified, if resource
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 97acba712e4e..bec1bef6f326 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -191,6 +191,88 @@ void __iomem *pci_ioremap_wc_bar(struct pci_dev *pdev, int bar)
> EXPORT_SYMBOL_GPL(pci_ioremap_wc_bar);
> #endif
>
> +/**
> + * pci_dev_str_match - test if a string matches a device
> + * @dev: the PCI device to test
> + * @p: string to match the device against
> + * @endptr: pointer to the string after the match
> + *
> + * Test if a string (typically from a kernel parameter) matches a
> + * specified. The string may be of one of two forms formats:
> + *
> + * [<domain>:]<bus>:<slot>.<func>
> + * pci:<vendor>:<device>[:<subvendor>:<subdevice>]
> + *
> + * The first format specifies a PCI bus/slot/function address which
> + * may change if new hardware is inserted, if motherboard firmware changes,
> + * or due to changes caused in kernel parameters.
> + *
> + * The second format matches devices using IDs in the configuration
> + * space which may match multiple devices in the system. A value of 0
> + * for any field will match all devices.
I realize this is not a change in behavior, but since we're spelling it
out in a proper comment rather than burying it in the implementation,
using 0 as a wildcard is rather questionable behavior. It always
surprises me when I read this because pci_match_one_device() uses
PCI_ANY_ID (~0) as a wildcard and as a result of struct pci_device_id
using __u32 for these fields, we actually need to specify ffffffff on
the commandline to get a wildcard match for dynamic ids. The latter is
tedious to use, but I think it's more correct, and the use of a __u32 is
probably attributed to the fact that 0xffff is only reserved for vendor
ID, the spec doesn't seem to reserve any entries from the vendor's
device ID range.
There's probably really no path to resolve these, but acknowledging the
difference in this comment block might be helpful in the future.
> + *
> + * Returns 1 if the string matches the device, 0 if it does not and
> + * a negative error code if the string cannot be parsed.
> + */
> +static int pci_dev_str_match(struct pci_dev *dev, const char *p,
> + const char **endptr)
> +{
> + int ret;
> + int seg, bus, slot, func, count;
> + unsigned short vendor, device, subsystem_vendor, subsystem_device;
> +
> + if (strncmp(p, "pci:", 4) == 0) {
> + /* PCI vendor/device (subvendor/subdevice) ids are specified */
> + p += 4;
> + ret = sscanf(p, "%hx:%hx:%hx:%hx%n", &vendor, &device,
> + &subsystem_vendor, &subsystem_device, &count);
> + if (ret != 4) {
> + ret = sscanf(p, "%hx:%hx%n", &vendor, &device, &count);
> + if (ret != 2)
> + return -EINVAL;
> +
> + subsystem_vendor = 0;
> + subsystem_device = 0;
> + }
> +
> + p += count;
> +
> + if ((!vendor || vendor == dev->vendor) &&
> + (!device || device == dev->device) &&
> + (!subsystem_vendor ||
> + subsystem_vendor == dev->subsystem_vendor) &&
> + (!subsystem_device ||
> + subsystem_device == dev->subsystem_device))
> + goto found;
> +
> + } else {
> + /* PCI Bus,Slot,Function ids are specified */
> + ret = sscanf(p, "%x:%x:%x.%x%n", &seg, &bus, &slot,
> + &func, &count);
> + if (ret != 4) {
> + seg = 0;
> + ret = sscanf(p, "%x:%x.%x%n", &bus, &slot,
> + &func, &count);
> + if (ret != 3)
> + return -EINVAL;
> + }
> +
> + p += count;
> +
> + if (seg == pci_domain_nr(dev->bus) &&
> + bus == dev->bus->number &&
> + slot == PCI_SLOT(dev->devfn) &&
> + func == PCI_FUNC(dev->devfn))
> + goto found;
> + }
> +
> + *endptr = p;
> + return 0;
> +
> +found:
> + *endptr = p;
> + return 1;
> +}
>
> static int __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
> u8 pos, int cap, int *ttl)
> @@ -5454,10 +5536,10 @@ static DEFINE_SPINLOCK(resource_alignment_lock);
> static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
> bool *resize)
> {
> - int seg, bus, slot, func, align_order, count;
> - unsigned short vendor, device, subsystem_vendor, subsystem_device;
> + int align_order, count;
> resource_size_t align = pcibios_default_alignment();
> - char *p;
> + const char *p;
> + int ret;
>
> spin_lock(&resource_alignment_lock);
> p = resource_alignment_param;
> @@ -5477,58 +5559,21 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
> } else {
> align_order = -1;
> }
> - if (strncmp(p, "pci:", 4) == 0) {
> - /* PCI vendor/device (subvendor/subdevice) ids are specified */
> - p += 4;
> - if (sscanf(p, "%hx:%hx:%hx:%hx%n",
> - &vendor, &device, &subsystem_vendor, &subsystem_device, &count) != 4) {
> - if (sscanf(p, "%hx:%hx%n", &vendor, &device, &count) != 2) {
> - printk(KERN_ERR "PCI: Can't parse resource_alignment parameter: pci:%s\n",
> - p);
> - break;
> - }
> - subsystem_vendor = subsystem_device = 0;
> - }
> - p += count;
> - if ((!vendor || (vendor == dev->vendor)) &&
> - (!device || (device == dev->device)) &&
> - (!subsystem_vendor || (subsystem_vendor == dev->subsystem_vendor)) &&
> - (!subsystem_device || (subsystem_device == dev->subsystem_device))) {
> - *resize = true;
> - if (align_order == -1)
> - align = PAGE_SIZE;
> - else
> - align = 1 << align_order;
> - /* Found */
> - break;
> - }
> - }
> - else {
> - if (sscanf(p, "%x:%x:%x.%x%n",
> - &seg, &bus, &slot, &func, &count) != 4) {
> - seg = 0;
> - if (sscanf(p, "%x:%x.%x%n",
> - &bus, &slot, &func, &count) != 3) {
> - /* Invalid format */
> - printk(KERN_ERR "PCI: Can't parse resource_alignment parameter: %s\n",
> - p);
> - break;
> - }
> - }
> - p += count;
> - if (seg == pci_domain_nr(dev->bus) &&
> - bus == dev->bus->number &&
> - slot == PCI_SLOT(dev->devfn) &&
> - func == PCI_FUNC(dev->devfn)) {
> - *resize = true;
> - if (align_order == -1)
> - align = PAGE_SIZE;
> - else
> - align = 1 << align_order;
> - /* Found */
> - break;
> - }
> +
> + ret = pci_dev_str_match(dev, p, &p);
> + if (ret == 1) {
> + *resize = true;
> + if (align_order == -1)
> + align = PAGE_SIZE;
> + else
> + align = 1 << align_order;
> + break;
> + } else if (ret < 0) {
> + pr_info("PCI: Can't parse resource_alignment parameter: pci:%s\n",
The "pci:" prefix on %s doesn't make sense now, it was used above when
the pointer was already advanced past this token, now I believe it would
lead to "pci:pci:xxxx:yyyy" or "pci:xx:yy.z". Thanks,
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2] Documentation/sphinx: add "nodocs" directive
From: Jani Nikula @ 2018-06-18 20:01 UTC (permalink / raw)
To: Mike Rapoport, Jonathan Corbet; +Cc: Matthew Wilcox, linux-doc, Mike Rapoport
In-Reply-To: <1529328996-16247-2-git-send-email-rppt@linux.vnet.ibm.com>
On Mon, 18 Jun 2018, Mike Rapoport <rppt@linux.vnet.ibm.com> wrote:
> When kernel-doc:: specified in .rst document without explicit directives,
> it outputs both comment and DOC: sections. If a DOC: section was explictly
> included in the same document it will be duplicated. For example, the
> output generated for Documentation/core-api/idr.rst [1] has "IDA
> description" in the "IDA usage" section and in the middle of the API
> reference.
>
> Addition of "nodocs" directive prevents the duplication without the need to
> explicitly define what functions should be include in the API reference.
>
> [1] https://www.kernel.org/doc/html/v4.17/core-api/idr.html
>
> Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
> ---
> Documentation/sphinx/kerneldoc.py | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
> index fbedcc3..bc5dd05 100644
> --- a/Documentation/sphinx/kerneldoc.py
> +++ b/Documentation/sphinx/kerneldoc.py
> @@ -50,6 +50,7 @@ class KernelDocDirective(Directive):
> 'functions': directives.unchanged_required,
> 'export': directives.unchanged,
> 'internal': directives.unchanged,
> + 'nodocs': directives.unchanged,
I'm not convinved this is the prettiest way to achieve what you
want. 'nodocs' seems kind of clunky.
I'd suggest supporting 'functions' without option arguments, and turning
that into kernel-doc -no-doc-sections.
The usage in patch 2/2 would turn into:
.. kernel-doc:: include/linux/idr.h
:functions:
which I think is much better overall in the rst source, complementing
the places where you use :doc:.
BR,
Jani.
> }
> has_content = False
>
> @@ -77,6 +78,8 @@ class KernelDocDirective(Directive):
> elif 'functions' in self.options:
> for f in str(self.options.get('functions')).split():
> cmd += ['-function', f]
> + elif 'nodocs' in self.options:
> + cmd += ['-no-doc-sections']
>
> for pattern in export_file_patterns:
> for f in glob.glob(env.config.kerneldoc_srctree + '/' + pattern):
--
Jani Nikula, Intel Open Source Graphics Center
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: overlayfs: caller_credentials option bypass creator_cred
From: Vivek Goyal @ 2018-06-18 19:43 UTC (permalink / raw)
To: Mark Salyzyn
Cc: linux-kernel, Miklos Szeredi, Jonathan Corbet, linux-unionfs,
linux-doc, Daniel Walsh, Stephen Smalley
In-Reply-To: <f141f883-0dbd-10ea-83b9-a1b38119b0c5@android.com>
On Mon, Jun 18, 2018 at 12:18:25PM -0700, Mark Salyzyn wrote:
> On 06/18/2018 11:54 AM, Vivek Goyal wrote:
> > On Mon, Jun 18, 2018 at 08:42:15AM -0700, Mark Salyzyn wrote:
> > > All accesses to the lower filesystems reference the creator (mount)
> > > and not the source context. This is a security issue.
> > Can you elaborate with an example that how this is a security issue.
> > mounter's check is in addition to caller's check. So we have two
> > checks in ovl_permission(). overlay inode gets the credentials from
> > underlying inode and we first check if caller is allowed to the
> > operation and if that's allowed, then we check if mounter is allowed
> > to do the operation.
> init which does the mount and represents the creator_cred which is granted a
> restricted MAC to do just what it needs to do, eg mount, but not be able to
> access the files. The caller comes in and is rejected because init domain is
> not allowed, even though the caller's domain is. MAC does not require
> overlap in privileges between the creator and the user.
[ CC dan walsh, stephen smalley ]
Ok. So this is not about security risk as such. It is more about that
it does not work in particular configuration where caller is allowed to
something but the mounter is not allowed to do that operation, so
operation fails.
(IIUC, recently this was raised as an issue if NFS is lower layer and
server is doing root squashing. Then root on client can do mount but might
not have access to a file while caller does have).
Will it be acceptable to write security policies in such a way so that
mounter has access as well.
Current model does assume that mounter has privileges on underlying files.
I think this works reasonably well with SELinux model of doing context
mounts. So if a "context=foo_label" mount is done, that changes labels
on overlay inodes (and ignore lables on disk). Mounter is the one allowing
this overriding. Now if we mounter allows this, then mounter should have
atleast access to underlying files. Otherwise this becomes equivalent
of that as long as mounter can do mount then it is providing access to
all files on disk/dir (with context mount). And mounter itself might not
be able to do those operations.
Core idea is, trying to make sure mounter of overlay does not provide
more priviliges to caller through a mount point if mounter itself does
not have privilege to do certain operations.
Another place it works well is that one does not have to provide
read/write labels on lower file. So in case of containers, SELinux can
just provide read labels to files in shared image dir, and as long
as mounter has read permission, it can copy up file and provide a writable
copy to the client. If we don't do mounter's check, that means caller
needs to have write permission to lower layer as well. And if caller
breaks out of container, it could directly write lower layer and attcking
another containers. So this was another use case in mind while coming
up with this model.
I am not saying that current model is the best way of doing things. Just
giving some context about why we thought it is a good idea.
Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [PATCH] net: fix e1000.rst Documentation build errors
From: Brown, Aaron F @ 2018-06-18 19:42 UTC (permalink / raw)
To: Randy Dunlap, netdev@vger.kernel.org, David Miller,
linux-doc@vger.kernel.org
Cc: LKML, Kirsher, Jeffrey T
In-Reply-To: <4e7a8aa5-cd19-3f57-cc60-6e6224d02a69@infradead.org>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2887 bytes --]
> From: Randy Dunlap [mailto:rdunlap@infradead.org]
> Sent: Saturday, June 16, 2018 5:36 PM
> To: netdev@vger.kernel.org; David Miller <davem@davemloft.net>; linux-
> doc@vger.kernel.org
> Cc: LKML <linux-kernel@vger.kernel.org>; Kirsher, Jeffrey T
> <jeffrey.t.kirsher@intel.com>; Brown, Aaron F <aaron.f.brown@intel.com>
> Subject: [PATCH] net: fix e1000.rst Documentation build errors
>
> From: Randy Dunlap <rdunlap@infradead.org>
>
> Fix Documentation build errors in e1000.rst. Several section titles and
> their underlines should not be indented.
>
> Documentation/networking/e1000.rst:358: (SEVERE/4) Unexpected section
> title.
>
> Fixes: 228046e76189 ("Documentation: e1000: Update kernel
> documentation")
>
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Aaron Brown <aaron.f.brown@intel.com>
> ---
> Is there a Sphinx version problem here? Tested-by: should indicate
> that there was no error like I am seeing.
The "Tested-by:" for this (and the e100 variant) was entirely focused on correctness of the documentation text, parameters match the driver, URLs were correct, etc... In retrospect "Tested-by:" is not exactly accurate and I probably should have left it at a simpler ack.
>
> Documentation/networking/e1000.rst | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> --- lnx-418-rc1.orig/Documentation/networking/e1000.rst
> +++ lnx-418-rc1/Documentation/networking/e1000.rst
> @@ -354,8 +354,8 @@ previously mentioned to force the adapte
> Additional Configurations
> =========================
>
> - Jumbo Frames
> - ------------
> +Jumbo Frames
> +------------
> Jumbo Frames support is enabled by changing the MTU to a value larger
> than
> the default of 1500. Use the ifconfig command to increase the MTU size.
> For example::
> @@ -389,8 +389,8 @@ Additional Configurations
> Intel(R) PRO/1000 Gigabit Server Adapter
> Intel(R) PRO/1000 PM Network Connection
>
> - ethtool
> - -------
> +ethtool
> +-------
> The driver utilizes the ethtool interface for driver configuration and
> diagnostics, as well as displaying statistical information. The ethtool
> version 1.6 or later is required for this functionality.
> @@ -398,8 +398,8 @@ Additional Configurations
> The latest release of ethtool can be found from
> https://www.kernel.org/pub/software/network/ethtool/
>
> - Enabling Wake on LAN* (WoL)
> - ---------------------------
> +Enabling Wake on LAN* (WoL)
> +---------------------------
> WoL is configured through the ethtool* utility.
>
> WoL will be enabled on the system during the next shut down or reboot.
>
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±v‡"žØ^n‡r¡ö¦zË\x1aëh™¨èÚ&¢ø\x1e®G«éh®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš
^ permalink raw reply
* [PATCH v3 2/3] PCI: Allow specifying devices using a base bus and path of devfns
From: Logan Gunthorpe @ 2018-06-18 19:36 UTC (permalink / raw)
To: linux-kernel, linux-pci, linux-doc
Cc: Stephen Bates, Christoph Hellwig, Bjorn Helgaas, Jonathan Corbet,
Ingo Molnar, Thomas Gleixner, Paul E. McKenney, Marc Zyngier,
Kai-Heng Feng, Frederic Weisbecker, Dan Williams,
Jérôme Glisse, Benjamin Herrenschmidt, Alex Williamson,
Christian König, Logan Gunthorpe
In-Reply-To: <20180618193636.16210-1-logang@deltatee.com>
When specifying PCI devices on the kernel command line using a
BDF, the bus numbers can change when adding or replacing a device,
changing motherboard firmware, or applying kernel parameters like
pci=assign-buses. When this happens, it is usually undesirable to
apply whatever command line tweak to the wrong device.
Therefore, it is useful to be able to specify devices with a base
bus number and the path of devfns needed to get to it. (Similar to
the "device scope" structure in the Intel VT-d spec, Section 8.3.1.)
Thus, we add an option to specify devices in the following format:
[<domain>:]<bus>:<slot>.<func>[/<slot>.<func>][/ ...]
The path can be any segment within the PCI hierarchy of any length and
determined through the use of 'lspci -t'. When specified this way, it is
less likely that a renumbered bus will result in a valid device specification
and the tweak won't be applied to the wrong device.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Stephen Bates <sbates@raithlin.com>
Acked-by: Christian König <christian.koenig@amd.com>
---
Documentation/admin-guide/kernel-parameters.txt | 8 +-
drivers/pci/pci.c | 120 ++++++++++++++++++++----
2 files changed, 106 insertions(+), 22 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 760fb2b0b349..d45285e1ab6a 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3000,14 +3000,18 @@
or a set of devices (<pci_dev>). These are
specified in one of the following formats:
- [<domain>:]<bus>:<slot>.<func>
+ [<domain>:]<bus>:<slot>.<func>[/<slot>.<func>][/ ...]
pci:<vendor>:<device>[:<subvendor>:<subdevice>]
Note: the first format specifies a PCI
bus/slot/function address which may change
if new hardware is inserted, if motherboard
firmware changes, or due to changes caused
- by other kernel parameters. The second format
+ by other kernel parameters. Optionally
+ a path from a device through multiple
+ slot/function addresses can be specified
+ after the base address (this is more robust
+ against renumbering issues). The second format
selects devices using IDs from the
configuration space which may match multiple
devices in the system.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bec1bef6f326..6fbad0492461 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -192,22 +192,111 @@ EXPORT_SYMBOL_GPL(pci_ioremap_wc_bar);
#endif
/**
+ * pci_dev_str_match_path - test if a path string matches a device
+ * @dev: the PCI device to test
+ * @p: string to match the device against
+ * @endptr: pointer to the string after the match
+ *
+ * Test if a string (typically from a kernel parameter) formated as a
+ * path of slot/function addresses matches a PCI device. The string must
+ * be of the form:
+ *
+ * [<domain>:]<bus>:<slot>.<func>/<slot>.<func>[/ ...]
+ *
+ * A path for a device can be obtained using 'lspci -t'. Using a path
+ * is more robust against renumbering of devices than using only
+ * a single bus, slot and function address.
+ *
+ * Returns 1 if the string matches the device, 0 if it does not and
+ * a negative error code if it fails to parse the string.
+ */
+static int pci_dev_str_match_path(struct pci_dev *dev, const char *path,
+ const char **endptr)
+{
+ int ret;
+ int seg, bus, slot, func;
+ char *wpath, *p;
+ char end;
+
+ *endptr = strchrnul(path, ';');
+
+ wpath = kmemdup_nul(path, *endptr - path, GFP_KERNEL);
+ if (!wpath)
+ return -ENOMEM;
+
+ while (1) {
+ p = strrchr(wpath, '/');
+ if (!p)
+ break;
+ ret = sscanf(p, "/%x.%x%c", &slot, &func, &end);
+ if (ret != 2) {
+ ret = -EINVAL;
+ goto free_and_exit;
+ }
+
+ if (dev->devfn != PCI_DEVFN(slot, func)) {
+ ret = 0;
+ goto free_and_exit;
+ }
+
+ /*
+ * Note: we don't need to get a reference to the upstream
+ * bridge because we hold a reference to the top level
+ * device which should hold a reference to the bridge,
+ * and so on.
+ */
+ dev = pci_upstream_bridge(dev);
+ if (!dev) {
+ ret = 0;
+ goto free_and_exit;
+ }
+
+ *p = 0;
+ }
+
+ ret = sscanf(wpath, "%x:%x:%x.%x%c", &seg, &bus, &slot,
+ &func, &end);
+ if (ret != 4) {
+ seg = 0;
+ ret = sscanf(wpath, "%x:%x.%x%c", &bus, &slot, &func, &end);
+ if (ret != 3) {
+ ret = -EINVAL;
+ goto free_and_exit;
+ }
+ }
+
+ ret = (seg == pci_domain_nr(dev->bus) &&
+ bus == dev->bus->number &&
+ dev->devfn == PCI_DEVFN(slot, func));
+
+free_and_exit:
+ kfree(wpath);
+ return ret;
+}
+
+/**
* pci_dev_str_match - test if a string matches a device
* @dev: the PCI device to test
* @p: string to match the device against
* @endptr: pointer to the string after the match
*
* Test if a string (typically from a kernel parameter) matches a
- * specified. The string may be of one of two forms formats:
+ * specified. The string may be of one of three formats:
*
* [<domain>:]<bus>:<slot>.<func>
+ * path:[<domain>:]<bus>:<slot>.<func>/<slot>.<func>[/ ...]
* pci:<vendor>:<device>[:<subvendor>:<subdevice>]
*
* The first format specifies a PCI bus/slot/function address which
* may change if new hardware is inserted, if motherboard firmware changes,
* or due to changes caused in kernel parameters.
*
- * The second format matches devices using IDs in the configuration
+ * The second format specifies a PCI bus/slot/function root address and
+ * a path of slot/function addresses to the specific device from the root.
+ * The path for a device can be determined through the use of 'lspci -t'.
+ * This format is more robust against renumbering issues than the first format.
+
+ * The third format matches devices using IDs in the configuration
* space which may match multiple devices in the system. A value of 0
* for any field will match all devices.
*
@@ -218,7 +307,7 @@ static int pci_dev_str_match(struct pci_dev *dev, const char *p,
const char **endptr)
{
int ret;
- int seg, bus, slot, func, count;
+ int count;
unsigned short vendor, device, subsystem_vendor, subsystem_device;
if (strncmp(p, "pci:", 4) == 0) {
@@ -244,25 +333,16 @@ static int pci_dev_str_match(struct pci_dev *dev, const char *p,
(!subsystem_device ||
subsystem_device == dev->subsystem_device))
goto found;
-
} else {
- /* PCI Bus,Slot,Function ids are specified */
- ret = sscanf(p, "%x:%x:%x.%x%n", &seg, &bus, &slot,
- &func, &count);
- if (ret != 4) {
- seg = 0;
- ret = sscanf(p, "%x:%x.%x%n", &bus, &slot,
- &func, &count);
- if (ret != 3)
- return -EINVAL;
- }
-
- p += count;
+ /*
+ * PCI Bus,Slot,Function ids are specified
+ * (optionally, may include a path of devfns following it)
+ */
- if (seg == pci_domain_nr(dev->bus) &&
- bus == dev->bus->number &&
- slot == PCI_SLOT(dev->devfn) &&
- func == PCI_FUNC(dev->devfn))
+ ret = pci_dev_str_match_path(dev, p, &p);
+ if (ret < 0)
+ return ret;
+ else if (ret)
goto found;
}
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v3 0/3] Add parameter for disabling ACS redirection for P2P
From: Logan Gunthorpe @ 2018-06-18 19:36 UTC (permalink / raw)
To: linux-kernel, linux-pci, linux-doc
Cc: Stephen Bates, Christoph Hellwig, Bjorn Helgaas, Jonathan Corbet,
Ingo Molnar, Thomas Gleixner, Paul E. McKenney, Marc Zyngier,
Kai-Heng Feng, Frederic Weisbecker, Dan Williams,
Jérôme Glisse, Benjamin Herrenschmidt, Alex Williamson,
Christian König, Logan Gunthorpe
Hi,
As discussed in our PCI P2PDMA series, we'd like to add a kernel
parameter for selectively disabling ACS redirection for select
bridges. Seeing this turned out to be a small series in itself, we've
decided to send this separately from the P2P work.
This series generalizes the code already done for the resource_alignment
option that already exists. The first patch creates a helper function
to match PCI devices against strings based on the code that already
existed in pci_specified_resource_alignment().
The second patch expands the new helper to optionally take a path of
PCI devfns. This is to address Alex's renumbering concern when using
simple bus-devfns. The implementation is essentially how he described it and
similar to the Intel VT-d spec (Section 8.3.1).
The final patch adds the disable_acs_redir kernel parameter which takes
a list of PCI devices and will disable the ACS P2P Request Redirect,
ACS P2P Completion Redirect and ACS P2P Egress Control bits for the
selected devices. This allows P2P traffic between selected bridges and
seeing it's done at boot, before IOMMU group creating the IOMMU groups
will be created correctly based on the bits.
Thanks,
Logan
--
Changes since v2:
* Rebased onto v4.18-rc1 (no conflicts)
* Minor tweaks to the documentation per Andy
* Removed the "path:" prefix and use the path parsing code
for simple devices (as it works the same). Per a suggestion from Alex
Changes since v1:
* Reworked pci_dev_str_match_path using strrchr as suggested by Alex
* Collected Christian's Acks
--
Logan Gunthorpe (3):
PCI: Make specifying PCI devices in kernel parameters reusable
PCI: Allow specifying devices using a base bus and path of devfns
PCI: Introduce the disable_acs_redir parameter
Documentation/admin-guide/kernel-parameters.txt | 39 ++-
drivers/pci/pci.c | 336 ++++++++++++++++++++----
2 files changed, 314 insertions(+), 61 deletions(-)
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v3 3/3] PCI: Introduce the disable_acs_redir parameter
From: Logan Gunthorpe @ 2018-06-18 19:36 UTC (permalink / raw)
To: linux-kernel, linux-pci, linux-doc
Cc: Stephen Bates, Christoph Hellwig, Bjorn Helgaas, Jonathan Corbet,
Ingo Molnar, Thomas Gleixner, Paul E. McKenney, Marc Zyngier,
Kai-Heng Feng, Frederic Weisbecker, Dan Williams,
Jérôme Glisse, Benjamin Herrenschmidt, Alex Williamson,
Christian König, Logan Gunthorpe
In-Reply-To: <20180618193636.16210-1-logang@deltatee.com>
In order to support P2P traffic on a segment of the PCI hierarchy,
we must be able to disable the ACS redirect bits for select
PCI bridges. The bridges must be selected before the devices are
discovered by the kernel and the IOMMU groups created. Therefore,
a kernel command line parameter is created to specify devices
which must have their ACS bits disabled.
The new parameter takes a list of devices separated by a semicolon.
Each device specified will have it's ACS redirect bits disabled.
This is similar to the existing 'resource_alignment' parameter and just
like it we also create a sysfs bus attribute which can be used to
read the parameter. Writing the parameter is not supported
as it would require forcibly hot plugging the affected device as
well as all devices whose IOMMU groups might change.
The ACS Request P2P Request Redirect, P2P Completion Redirect and P2P
Egress Control bits are disabled which is sufficient to always allow
passing P2P traffic uninterrupted. The bits are set after the kernel
(optionally) enables the ACS bits itself. It is also done regardless of
whether the kernel sets the bits or not seeing some BIOS firmware is known
to set the bits on boot.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Stephen Bates <sbates@raithlin.com>
Acked-by: Christian König <christian.koenig@amd.com>
---
Documentation/admin-guide/kernel-parameters.txt | 9 +++
drivers/pci/pci.c | 103 +++++++++++++++++++++++-
2 files changed, 110 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index d45285e1ab6a..2ec36e258bb0 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3190,6 +3190,15 @@
Adding the window is slightly risky (it may
conflict with unreported devices), so this
taints the kernel.
+ disable_acs_redir=<pci_dev>[; ...]
+ Specify one or more PCI devices (in the format
+ specified above) separated by semicolons.
+ Each device specified will have the PCI ACS
+ redirect capabilities forced off which will
+ allow P2P traffic between devices through
+ bridges without forcing it upstream. Note:
+ this removes isolation between devices and
+ will make the IOMMU groups less granular.
pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
Management.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6fbad0492461..eb85bf507398 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2982,6 +2982,92 @@ void pci_request_acs(void)
pci_acs_enable = 1;
}
+#define DISABLE_ACS_REDIR_PARAM_SIZE COMMAND_LINE_SIZE
+static char disable_acs_redir_param[DISABLE_ACS_REDIR_PARAM_SIZE] = {0};
+static DEFINE_SPINLOCK(disable_acs_redir_lock);
+
+static ssize_t pci_set_disable_acs_redir_param(const char *buf, size_t count)
+{
+ if (count > DISABLE_ACS_REDIR_PARAM_SIZE - 1)
+ count = DISABLE_ACS_REDIR_PARAM_SIZE - 1;
+ spin_lock(&disable_acs_redir_lock);
+ strncpy(disable_acs_redir_param, buf, count);
+ disable_acs_redir_param[count] = '\0';
+ spin_unlock(&disable_acs_redir_lock);
+ return count;
+}
+
+static ssize_t pci_disable_acs_redir_show(struct bus_type *bus, char *buf)
+{
+ size_t count;
+
+ spin_lock(&disable_acs_redir_lock);
+ count = snprintf(buf, PAGE_SIZE, "%s\n", disable_acs_redir_param);
+ spin_unlock(&disable_acs_redir_lock);
+ return count;
+}
+
+static BUS_ATTR(disable_acs_redir, 0444, pci_disable_acs_redir_show, NULL);
+
+static int __init pci_disable_acs_redir_sysfs_init(void)
+{
+ return bus_create_file(&pci_bus_type, &bus_attr_disable_acs_redir);
+}
+late_initcall(pci_disable_acs_redir_sysfs_init);
+
+/**
+ * pci_disable_acs_redir - disable ACS redirect capabilities
+ * @dev: the PCI device
+ *
+ * For only devices specified in the disable_acs_redir parameter.
+ */
+static void pci_disable_acs_redir(struct pci_dev *dev)
+{
+ int ret = 0;
+ const char *p;
+ int pos;
+ u16 ctrl;
+
+ spin_lock(&disable_acs_redir_lock);
+
+ p = disable_acs_redir_param;
+ while (*p) {
+ ret = pci_dev_str_match(dev, p, &p);
+ if (ret < 0) {
+ pr_info_once("PCI: Can't parse disable_acs_redir parameter: %s\n",
+ disable_acs_redir_param);
+
+ break;
+ } else if (ret == 1) {
+ /* Found a match */
+ break;
+ }
+
+ if (*p != ';' && *p != ',') {
+ /* End of param or invalid format */
+ break;
+ }
+ p++;
+ }
+ spin_unlock(&disable_acs_redir_lock);
+
+ if (ret != 1)
+ return;
+
+ pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+ if (!pos)
+ return;
+
+ pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
+
+ /* P2P Request & Completion Redirect */
+ ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC);
+
+ pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+
+ pci_info(dev, "disabled ACS redirect\n");
+}
+
/**
* pci_std_enable_acs - enable ACS on devices using standard ACS capabilites
* @dev: the PCI device
@@ -3021,12 +3107,22 @@ static void pci_std_enable_acs(struct pci_dev *dev)
void pci_enable_acs(struct pci_dev *dev)
{
if (!pci_acs_enable)
- return;
+ goto disable_acs_redir;
if (!pci_dev_specific_enable_acs(dev))
- return;
+ goto disable_acs_redir;
pci_std_enable_acs(dev);
+
+disable_acs_redir:
+ /*
+ * Note: pci_disable_acs_redir() must be called even if
+ * ACS is not enabled by the kernel because the firmware
+ * may have unexpectedly set the flags. So if we are told
+ * to disable it, we should always disable it after setting
+ * the kernel's default preferences.
+ */
+ pci_disable_acs_redir(dev);
}
static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags)
@@ -5966,6 +6062,9 @@ static int __init pci_setup(char *str)
pcie_bus_config = PCIE_BUS_PEER2PEER;
} else if (!strncmp(str, "pcie_scan_all", 13)) {
pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
+ } else if (!strncmp(str, "disable_acs_redir=", 18)) {
+ pci_set_disable_acs_redir_param(str + 18,
+ strlen(str + 18));
} else {
printk(KERN_ERR "PCI: Unknown option `%s'\n",
str);
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox