* Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
From: Alexander Potapenko @ 2017-03-17 12:32 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Michael Davidson, Michal Marek, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Herbert Xu, David S. Miller, Shaohua Li,
Dmitry Vyukov, Matthias Kaehlcke, x86, linux-kbuild, LKML,
linux-crypto, linux-raid
In-Reply-To: <CAG_fn=XcQLtcHRSm2BHYce9LMtyFJ+fWxfE4vYPCw+7U0DcrmQ@mail.gmail.com>
On Fri, Mar 17, 2017 at 1:31 PM, Alexander Potapenko <glider@google.com> wrote:
> On Fri, Mar 17, 2017 at 1:08 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> On Thu, Mar 16, 2017 at 05:15:19PM -0700, Michael Davidson wrote:
>>> Replace a variable length array in a struct by allocating
>>> the memory for the entire struct in a char array on the stack.
>>>
>>> Signed-off-by: Michael Davidson <md@google.com>
>>> ---
>>> drivers/md/raid10.c | 9 ++++-----
>>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>>> index 063c43d83b72..158ebdff782c 100644
>>> --- a/drivers/md/raid10.c
>>> +++ b/drivers/md/raid10.c
>>> @@ -4654,11 +4654,10 @@ static int handle_reshape_read_error(struct mddev *mddev,
>>> /* Use sync reads to get the blocks from somewhere else */
>>> int sectors = r10_bio->sectors;
>>> struct r10conf *conf = mddev->private;
>>> - struct {
>>> - struct r10bio r10_bio;
>>> - struct r10dev devs[conf->copies];
>>> - } on_stack;
>>> - struct r10bio *r10b = &on_stack.r10_bio;
>>> + char on_stack_r10_bio[sizeof(struct r10bio) +
>>> + conf->copies * sizeof(struct r10dev)]
>>> + __aligned(__alignof__(struct r10bio));
>>> + struct r10bio *r10b = (struct r10bio *)on_stack_r10_bio;
>>> int slot = 0;
>>> int idx = 0;
>>> struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
>>
>>
>> That's disgusting. Why not fix LLVM to support this?
>
> IIUC there's only a handful of VLAIS instances in LLVM code, why not
Sorry, "kernel code", not "LLVM code".
> just drop them for the sake of better code portability?
> (To quote Linus, "this feature is an abomination":
> https://lkml.org/lkml/2013/9/23/500)
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
From: Peter Zijlstra @ 2017-03-17 12:44 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Michael Davidson, Michal Marek, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Herbert Xu, David S. Miller, Shaohua Li,
Dmitry Vyukov, Matthias Kaehlcke, x86, linux-kbuild, LKML,
linux-crypto, linux-raid
In-Reply-To: <CAG_fn=XcQLtcHRSm2BHYce9LMtyFJ+fWxfE4vYPCw+7U0DcrmQ@mail.gmail.com>
On Fri, Mar 17, 2017 at 01:31:23PM +0100, Alexander Potapenko wrote:
> On Fri, Mar 17, 2017 at 1:08 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Thu, Mar 16, 2017 at 05:15:19PM -0700, Michael Davidson wrote:
> >> Replace a variable length array in a struct by allocating
> >> the memory for the entire struct in a char array on the stack.
> >>
> >> Signed-off-by: Michael Davidson <md@google.com>
> >> ---
> >> drivers/md/raid10.c | 9 ++++-----
> >> 1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> >> index 063c43d83b72..158ebdff782c 100644
> >> --- a/drivers/md/raid10.c
> >> +++ b/drivers/md/raid10.c
> >> @@ -4654,11 +4654,10 @@ static int handle_reshape_read_error(struct mddev *mddev,
> >> /* Use sync reads to get the blocks from somewhere else */
> >> int sectors = r10_bio->sectors;
> >> struct r10conf *conf = mddev->private;
> >> - struct {
> >> - struct r10bio r10_bio;
> >> - struct r10dev devs[conf->copies];
> >> - } on_stack;
> >> - struct r10bio *r10b = &on_stack.r10_bio;
> >> + char on_stack_r10_bio[sizeof(struct r10bio) +
> >> + conf->copies * sizeof(struct r10dev)]
> >> + __aligned(__alignof__(struct r10bio));
> >> + struct r10bio *r10b = (struct r10bio *)on_stack_r10_bio;
> >> int slot = 0;
> >> int idx = 0;
> >> struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
> >
> >
> > That's disgusting. Why not fix LLVM to support this?
>
> IIUC there's only a handful of VLAIS instances in LLVM code, why not
> just drop them for the sake of better code portability?
> (To quote Linus, "this feature is an abomination":
> https://lkml.org/lkml/2013/9/23/500)
Be that as it may; what you construct above is disgusting. Surely the
code can be refactored to not look like dog vomit?
Also; its not immediately obvious conf->copies is 'small' and this
doesn't blow up the stack; I feel that deserves a comment somewhere.
^ permalink raw reply
* Re: [PATCH] dt-bindings: rng: clocks property on omap_rng is optional
From: Thomas Petazzoni @ 2017-03-17 12:57 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Ian Campbell, Pawel Moll,
Mark Rutland, Kumar Gala, Deepak Saxena, Matt Mackall, Herbert Xu,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
romain.perier-ZGY8ohtN/8qB+jHODAdFcQ, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Gregory Clement, Nadav Haklai, Hanna Hawa,
Yehuda Yitschak, stable-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170315175737.vfy5omissjkyyie7@rob-hp-laptop>
Hello,
On Wed, 15 Mar 2017 12:57:37 -0500, Rob Herring wrote:
> > diff --git a/Documentation/devicetree/bindings/rng/omap_rng.txt b/Documentation/devicetree/bindings/rng/omap_rng.txt
> > index 4714772..20d435da 100644
> > --- a/Documentation/devicetree/bindings/rng/omap_rng.txt
> > +++ b/Documentation/devicetree/bindings/rng/omap_rng.txt
> > @@ -12,6 +12,9 @@ Required properties:
> > - reg : Offset and length of the register set for the module
> > - interrupts : the interrupt number for the RNG module.
> > Used for "ti,omap4-rng" and "inside-secure,safexcel-eip76"
> > +
> > +Optional properties:
> > +
>
> Wouldn't just "for ? compatible only" be more correct?
I don't know if at the HW point of view the EIP76 will *always* need a
clock. Maybe it depends on the integration in the SoC.
But anyway, let's mark the clocks property as mandatory for
"inside-secure,safexcel-eip76" for the moment, we can always relax this
requirement later on if we realize that some EIP76 have been integrated
in a way that doesn't require a clock.
It is worth mentioning that the actual driver implementation simply
makes the clock optional in all cases, without looking at the
compatible to figure out if the clock must be there or not. But that's
just the current driver implementation. The Device Tree binding
specification can be more specific than what the current driver does.
Therefore: v2 coming.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2] dt-bindings: rng: clocks property on omap_rng not always mandatory
From: Thomas Petazzoni @ 2017-03-17 12:58 UTC (permalink / raw)
To: devicetree, Rob Herring, Ian Campbell, Pawel Moll, Mark Rutland,
Kumar Gala
Cc: Herbert Xu, linux-crypto, romain.perier, Nadav Haklai, Hanna Hawa,
Yehuda Yitschak, Jason Cooper, Andrew Lunn, Sebastian Hesselbarth,
Gregory Clement, Thomas Petazzoni, stable
Commit 52060836f79 ("dt-bindings: omap-rng: Document SafeXcel IP-76
device variant") update the omap_rng Device Tree binding to add support
for the IP-76 variation of the IP. As part of this change, a "clocks"
property was added, but is indicated as "Required", without indicated
it's actually only required for some compatible strings.
This commit fixes that, by explicitly stating that the clocks property
is only required with the inside-secure,safexcel-eip76 compatible
string.
Fixes: 52060836f79 ("dt-bindings: omap-rng: Document SafeXcel IP-76 device variant")
Cc: <stable@vger.kernel.org>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
Changes since v1:
- Instead of indicating the property as optional, indicate it as
mandatory for the inside-secure,safexcel-eip76 compatible string, as
suggested by Rob Herring.
---
Documentation/devicetree/bindings/rng/omap_rng.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/rng/omap_rng.txt b/Documentation/devicetree/bindings/rng/omap_rng.txt
index 4714772..9cf7876 100644
--- a/Documentation/devicetree/bindings/rng/omap_rng.txt
+++ b/Documentation/devicetree/bindings/rng/omap_rng.txt
@@ -12,7 +12,8 @@ Required properties:
- reg : Offset and length of the register set for the module
- interrupts : the interrupt number for the RNG module.
Used for "ti,omap4-rng" and "inside-secure,safexcel-eip76"
-- clocks: the trng clock source
+- clocks: the trng clock source. Only mandatory for the
+ "inside-secure,safexcel-eip76" compatible.
Example:
/* AM335x */
--
2.7.4
^ permalink raw reply related
* Re: [RFC PATCH v2 08/32] x86: Use PAGE_KERNEL protection for ioremap of memory page
From: Tom Lendacky @ 2017-03-17 14:32 UTC (permalink / raw)
To: Borislav Petkov, Brijesh Singh
Cc: simon.guinot, linux-efi, kvm, rkrcmar, matt, linux-pci,
linus.walleij, gary.hook, linux-mm, paul.gortmaker, hpa, cl,
dan.j.williams, aarcange, sfr, andriy.shevchenko, herbert, bhe,
xemul, joro, x86, peterz, piotr.luc, mingo, msalter, ross.zwisler,
dyoung, jroedel, keescook, arnd, toshi.kani, mathieu.desnoyers,
luto, devel, bhelgaas, tglx, mchehab
In-Reply-To: <413f12e9-818a-745d-374b-3dbc439e972c@amd.com>
On 3/16/2017 3:04 PM, Tom Lendacky wrote:
> On 3/7/2017 8:59 AM, Borislav Petkov wrote:
>> On Thu, Mar 02, 2017 at 10:13:32AM -0500, Brijesh Singh wrote:
>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> In order for memory pages to be properly mapped when SEV is active, we
>>> need to use the PAGE_KERNEL protection attribute as the base protection.
>>> This will insure that memory mapping of, e.g. ACPI tables, receives the
>>> proper mapping attributes.
>>>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> ---
>>
>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>>> index c400ab5..481c999 100644
>>> --- a/arch/x86/mm/ioremap.c
>>> +++ b/arch/x86/mm/ioremap.c
>>> @@ -151,7 +151,15 @@ static void __iomem
>>> *__ioremap_caller(resource_size_t phys_addr,
>>> pcm = new_pcm;
>>> }
>>>
>>> + /*
>>> + * If the page being mapped is in memory and SEV is active then
>>> + * make sure the memory encryption attribute is enabled in the
>>> + * resulting mapping.
>>> + */
>>> prot = PAGE_KERNEL_IO;
>>> + if (sev_active() && page_is_mem(pfn))
>>
>> Hmm, a resource tree walk per ioremap call. This could get expensive for
>> ioremap-heavy workloads.
>>
>> __ioremap_caller() gets called here during boot 55 times so not a whole
>> lot but I wouldn't be surprised if there were some nasty use cases which
>> ioremap a lot.
>>
>> ...
>>
>>> diff --git a/kernel/resource.c b/kernel/resource.c
>>> index 9b5f044..db56ba3 100644
>>> --- a/kernel/resource.c
>>> +++ b/kernel/resource.c
>>> @@ -518,6 +518,46 @@ int __weak page_is_ram(unsigned long pfn)
>>> }
>>> EXPORT_SYMBOL_GPL(page_is_ram);
>>>
>>> +/*
>>> + * This function returns true if the target memory is marked as
>>> + * IORESOURCE_MEM and IORESOUCE_BUSY and described as other than
>>> + * IORES_DESC_NONE (e.g. IORES_DESC_ACPI_TABLES).
>>> + */
>>> +static int walk_mem_range(unsigned long start_pfn, unsigned long
>>> nr_pages)
>>> +{
>>> + struct resource res;
>>> + unsigned long pfn, end_pfn;
>>> + u64 orig_end;
>>> + int ret = -1;
>>> +
>>> + res.start = (u64) start_pfn << PAGE_SHIFT;
>>> + res.end = ((u64)(start_pfn + nr_pages) << PAGE_SHIFT) - 1;
>>> + res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>>> + orig_end = res.end;
>>> + while ((res.start < res.end) &&
>>> + (find_next_iomem_res(&res, IORES_DESC_NONE, true) >= 0)) {
>>> + pfn = (res.start + PAGE_SIZE - 1) >> PAGE_SHIFT;
>>> + end_pfn = (res.end + 1) >> PAGE_SHIFT;
>>> + if (end_pfn > pfn)
>>> + ret = (res.desc != IORES_DESC_NONE) ? 1 : 0;
>>> + if (ret)
>>> + break;
>>> + res.start = res.end + 1;
>>> + res.end = orig_end;
>>> + }
>>> + return ret;
>>> +}
>>
>> So the relevant difference between this one and walk_system_ram_range()
>> is this:
>>
>> - ret = (*func)(pfn, end_pfn - pfn, arg);
>> + ret = (res.desc != IORES_DESC_NONE) ? 1 : 0;
>>
>> so it seems to me you can have your own *func() pointer which does that
>> IORES_DESC_NONE comparison. And then you can define your own workhorse
>> __walk_memory_range() which gets called by both walk_mem_range() and
>> walk_system_ram_range() instead of almost duplicating them.
>>
>> And looking at walk_system_ram_res(), that one looks similar too except
>> the pfn computation. But AFAICT the pfn/end_pfn things are computed from
>> res.start and res.end so it looks to me like all those three functions
>> are crying for unification...
>
> I'll take a look at what it takes to consolidate these with a pre-patch.
> Then I'll add the new support.
It looks pretty straight forward to combine walk_iomem_res_desc() and
walk_system_ram_res(). The walk_system_ram_range() function would fit
easily into this, also, except for the fact that the callback function
takes unsigned longs vs the u64s of the other functions. Is it worth
modifying all of the callers of walk_system_ram_range() (which are only
about 8 locations) to change the callback functions to accept u64s in
order to consolidate the walk_system_ram_range() function, too?
Thanks,
Tom
>
> Thanks,
> Tom
>
>>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [PATCH 0/4] crypto: s5p-sss - Fix and minor improvements
From: Krzysztof Kozlowski @ 2017-03-17 14:49 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, linux-crypto, linux-kernel,
Vladimir Zapolskiy
Cc: Nathan Royce, Krzysztof Kozlowski
Hi,
I still did not fix the NULL pointer dereference reported by
Nathan Royce [1], but I got some other improvements.
Testing done on Odroid U3 (Exynos4412) with tcrypt and cryptsetup.
Best regards,
Krzysztof
[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1351149.html
Krzysztof Kozlowski (4):
crypto: s5p-sss - Close possible race for completed requests
crypto: s5p-sss - Remove unused variant field from state container
crypto: s5p-sss - Document the struct s5p_aes_dev
crypto: s5p-sss - Use mutex instead of spinlock
drivers/crypto/s5p-sss.c | 70 +++++++++++++++++++++++++++++++-----------------
1 file changed, 45 insertions(+), 25 deletions(-)
--
2.9.3
^ permalink raw reply
* [PATCH 1/4] crypto: s5p-sss - Close possible race for completed requests
From: Krzysztof Kozlowski @ 2017-03-17 14:49 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, linux-crypto, linux-kernel,
Vladimir Zapolskiy
Cc: Nathan Royce, Krzysztof Kozlowski, # v4 . 10 . x
In-Reply-To: <20170317144922.27379-1-krzk@kernel.org>
Driver is capable of handling only one request at a time and it stores
it in its state container struct s5p_aes_dev. This stored request must be
protected between concurrent invocations (e.g. completing current
request and scheduling new one). Combination of lock and "busy" field
is used for that purpose.
When "busy" field is true, the driver will not accept new request thus
it will not overwrite currently handled data.
However commit 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion
on LRW(AES)") moved some of the write to "busy" field out of a lock
protected critical section. This might lead to potential race between
completing current request and scheduling a new one. Effectively the
request completion might try to operate on new crypto request.
Cc: <stable@vger.kernel.org> # v4.10.x
Fixes: 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion on LRW(AES)")
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
drivers/crypto/s5p-sss.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 1b9da3dc799b..6c620487e9c2 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -287,7 +287,6 @@ static void s5p_sg_done(struct s5p_aes_dev *dev)
static void s5p_aes_complete(struct s5p_aes_dev *dev, int err)
{
dev->req->base.complete(&dev->req->base, err);
- dev->busy = false;
}
static void s5p_unset_outdata(struct s5p_aes_dev *dev)
@@ -462,7 +461,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
spin_unlock_irqrestore(&dev->lock, flags);
s5p_aes_complete(dev, 0);
- dev->busy = true;
+ /* Device is still busy */
tasklet_schedule(&dev->tasklet);
} else {
/*
@@ -483,6 +482,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
error:
s5p_sg_done(dev);
+ dev->busy = false;
spin_unlock_irqrestore(&dev->lock, flags);
s5p_aes_complete(dev, err);
@@ -634,6 +634,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
indata_error:
s5p_sg_done(dev);
+ dev->busy = false;
spin_unlock_irqrestore(&dev->lock, flags);
s5p_aes_complete(dev, err);
}
--
2.9.3
^ permalink raw reply related
* [PATCH 3/4] crypto: s5p-sss - Document the struct s5p_aes_dev
From: Krzysztof Kozlowski @ 2017-03-17 14:49 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, linux-crypto, linux-kernel,
Vladimir Zapolskiy
Cc: Nathan Royce, Krzysztof Kozlowski
In-Reply-To: <20170317144922.27379-1-krzk@kernel.org>
Add kernel-doc to s5p_aes_dev structure.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
drivers/crypto/s5p-sss.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 35ea84b7d775..7ac657f46d15 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -170,6 +170,32 @@ struct s5p_aes_ctx {
int keylen;
};
+/**
+ * struct s5p_aes_dev - Crypto device state container
+ * @dev: Associated device
+ * @clk: Clock for accessing hardware
+ * @ioaddr: Mapped IO memory region
+ * @aes_ioaddr: Per-varian offset for AES block IO memory
+ * @irq_fc: Feed control interrupt line
+ * @req: Crypto request currently handled by the device
+ * @ctx: Configuration for currently handled crypto request
+ * @sg_src: Scatter list with source data for currently handled block
+ * in device. This is DMA-mapped into device.
+ * @sg_dst: Scatter list with destination data for currently handled block
+ * in device. This is DMA-mapped into device.
+ * @sg_src_cpy: In case of unaligned access, copied scatter list
+ * with source data.
+ * @sg_dst_cpy: In case of unaligned access, copied scatter list
+ * with destination data.
+ * @tasklet: New request scheduling jib
+ * @queue: Crypto queue
+ * @busy: Indicates whether the device is currently handling some request
+ * thus it uses some of the fields from this state, like:
+ * req, ctx, sg_src/dst (and copies). This essentially
+ * protects against concurrent access to these fields.
+ * @lock: Lock for protecting both access to device hardware registers
+ * and fields related to current request (including the busy field).
+ */
struct s5p_aes_dev {
struct device *dev;
struct clk *clk;
@@ -182,7 +208,6 @@ struct s5p_aes_dev {
struct scatterlist *sg_src;
struct scatterlist *sg_dst;
- /* In case of unaligned access: */
struct scatterlist *sg_src_cpy;
struct scatterlist *sg_dst_cpy;
--
2.9.3
^ permalink raw reply related
* [PATCH 4/4] crypto: s5p-sss - Use mutex instead of spinlock
From: Krzysztof Kozlowski @ 2017-03-17 14:49 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, linux-crypto, linux-kernel,
Vladimir Zapolskiy
Cc: Nathan Royce, Krzysztof Kozlowski
In-Reply-To: <20170317144922.27379-1-krzk@kernel.org>
Driver uses threaded interrupt handler so there is no real need for
using spinlocks for synchronization. Mutexes would do fine and are
friendlier for overall system preemptivness and real-time behavior.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
drivers/crypto/s5p-sss.c | 35 ++++++++++++++++-------------------
1 file changed, 16 insertions(+), 19 deletions(-)
diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 7ac657f46d15..1893cf5dedc0 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -21,6 +21,7 @@
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/scatterlist.h>
@@ -214,7 +215,7 @@ struct s5p_aes_dev {
struct tasklet_struct tasklet;
struct crypto_queue queue;
bool busy;
- spinlock_t lock;
+ struct mutex lock;
};
static struct s5p_aes_dev *s5p_dev;
@@ -443,11 +444,10 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
int err_dma_tx = 0;
int err_dma_rx = 0;
bool tx_end = false;
- unsigned long flags;
uint32_t status;
int err;
- spin_lock_irqsave(&dev->lock, flags);
+ mutex_lock(&dev->lock);
/*
* Handle rx or tx interrupt. If there is still data (scatterlist did not
@@ -481,7 +481,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
if (tx_end) {
s5p_sg_done(dev);
- spin_unlock_irqrestore(&dev->lock, flags);
+ mutex_unlock(&dev->lock);
s5p_aes_complete(dev, 0);
/* Device is still busy */
@@ -498,7 +498,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
if (err_dma_rx == 1)
s5p_set_dma_indata(dev, dev->sg_src);
- spin_unlock_irqrestore(&dev->lock, flags);
+ mutex_unlock(&dev->lock);
}
return IRQ_HANDLED;
@@ -506,7 +506,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
error:
s5p_sg_done(dev);
dev->busy = false;
- spin_unlock_irqrestore(&dev->lock, flags);
+ mutex_unlock(&dev->lock);
s5p_aes_complete(dev, err);
return IRQ_HANDLED;
@@ -599,7 +599,6 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
{
struct ablkcipher_request *req = dev->req;
uint32_t aes_control;
- unsigned long flags;
int err;
aes_control = SSS_AES_KEY_CHANGE_MODE;
@@ -625,7 +624,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
| SSS_AES_BYTESWAP_KEY
| SSS_AES_BYTESWAP_CNT;
- spin_lock_irqsave(&dev->lock, flags);
+ mutex_lock(&dev->lock);
SSS_WRITE(dev, FCINTENCLR,
SSS_FCINTENCLR_BTDMAINTENCLR | SSS_FCINTENCLR_BRDMAINTENCLR);
@@ -648,7 +647,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
SSS_WRITE(dev, FCINTENSET,
SSS_FCINTENSET_BTDMAINTENSET | SSS_FCINTENSET_BRDMAINTENSET);
- spin_unlock_irqrestore(&dev->lock, flags);
+ mutex_unlock(&dev->lock);
return;
@@ -658,7 +657,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
indata_error:
s5p_sg_done(dev);
dev->busy = false;
- spin_unlock_irqrestore(&dev->lock, flags);
+ mutex_unlock(&dev->lock);
s5p_aes_complete(dev, err);
}
@@ -667,18 +666,17 @@ static void s5p_tasklet_cb(unsigned long data)
struct s5p_aes_dev *dev = (struct s5p_aes_dev *)data;
struct crypto_async_request *async_req, *backlog;
struct s5p_aes_reqctx *reqctx;
- unsigned long flags;
- spin_lock_irqsave(&dev->lock, flags);
+ mutex_lock(&dev->lock);
backlog = crypto_get_backlog(&dev->queue);
async_req = crypto_dequeue_request(&dev->queue);
if (!async_req) {
dev->busy = false;
- spin_unlock_irqrestore(&dev->lock, flags);
+ mutex_unlock(&dev->lock);
return;
}
- spin_unlock_irqrestore(&dev->lock, flags);
+ mutex_unlock(&dev->lock);
if (backlog)
backlog->complete(backlog, -EINPROGRESS);
@@ -693,18 +691,17 @@ static void s5p_tasklet_cb(unsigned long data)
static int s5p_aes_handle_req(struct s5p_aes_dev *dev,
struct ablkcipher_request *req)
{
- unsigned long flags;
int err;
- spin_lock_irqsave(&dev->lock, flags);
+ mutex_lock(&dev->lock);
err = ablkcipher_enqueue_request(&dev->queue, req);
if (dev->busy) {
- spin_unlock_irqrestore(&dev->lock, flags);
+ mutex_unlock(&dev->lock);
goto exit;
}
dev->busy = true;
- spin_unlock_irqrestore(&dev->lock, flags);
+ mutex_unlock(&dev->lock);
tasklet_schedule(&dev->tasklet);
@@ -856,7 +853,7 @@ static int s5p_aes_probe(struct platform_device *pdev)
return err;
}
- spin_lock_init(&pdata->lock);
+ mutex_init(&pdata->lock);
pdata->aes_ioaddr = pdata->ioaddr + variant->aes_offset;
--
2.9.3
^ permalink raw reply related
* Re: [RFC PATCH v2 08/32] x86: Use PAGE_KERNEL protection for ioremap of memory page
From: Tom Lendacky @ 2017-03-17 14:55 UTC (permalink / raw)
To: Borislav Petkov, Brijesh Singh
Cc: linux-efi, kvm, rkrcmar, matt, linux-pci, linus.walleij,
gary.hook, linux-mm, hpa, cl, tglx, aarcange, sfr, mchehab,
simon.guinot, bhe, xemul, joro, x86, peterz, piotr.luc, mingo,
msalter, ross.zwisler, labbott, dyoung, jroedel, keescook, arnd,
toshi.kani, mathieu.desnoyers, luto, pbonzini, bhelgaas,
dan.j.williams, andriy.shevchenko, akpm, herbert, tony.luck,
paul.gortmaker
In-Reply-To: <0a7de265-1352-6327-ef3a-4287bfca732d@amd.com>
On 3/17/2017 9:32 AM, Tom Lendacky wrote:
> On 3/16/2017 3:04 PM, Tom Lendacky wrote:
>> On 3/7/2017 8:59 AM, Borislav Petkov wrote:
>>> On Thu, Mar 02, 2017 at 10:13:32AM -0500, Brijesh Singh wrote:
>>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>>
>>>> In order for memory pages to be properly mapped when SEV is active, we
>>>> need to use the PAGE_KERNEL protection attribute as the base
>>>> protection.
>>>> This will insure that memory mapping of, e.g. ACPI tables, receives the
>>>> proper mapping attributes.
>>>>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>> ---
>>>
>>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>>>> index c400ab5..481c999 100644
>>>> --- a/arch/x86/mm/ioremap.c
>>>> +++ b/arch/x86/mm/ioremap.c
>>>> @@ -151,7 +151,15 @@ static void __iomem
>>>> *__ioremap_caller(resource_size_t phys_addr,
>>>> pcm = new_pcm;
>>>> }
>>>>
>>>> + /*
>>>> + * If the page being mapped is in memory and SEV is active then
>>>> + * make sure the memory encryption attribute is enabled in the
>>>> + * resulting mapping.
>>>> + */
>>>> prot = PAGE_KERNEL_IO;
>>>> + if (sev_active() && page_is_mem(pfn))
>>>
>>> Hmm, a resource tree walk per ioremap call. This could get expensive for
>>> ioremap-heavy workloads.
>>>
>>> __ioremap_caller() gets called here during boot 55 times so not a whole
>>> lot but I wouldn't be surprised if there were some nasty use cases which
>>> ioremap a lot.
>>>
>>> ...
>>>
>>>> diff --git a/kernel/resource.c b/kernel/resource.c
>>>> index 9b5f044..db56ba3 100644
>>>> --- a/kernel/resource.c
>>>> +++ b/kernel/resource.c
>>>> @@ -518,6 +518,46 @@ int __weak page_is_ram(unsigned long pfn)
>>>> }
>>>> EXPORT_SYMBOL_GPL(page_is_ram);
>>>>
>>>> +/*
>>>> + * This function returns true if the target memory is marked as
>>>> + * IORESOURCE_MEM and IORESOUCE_BUSY and described as other than
>>>> + * IORES_DESC_NONE (e.g. IORES_DESC_ACPI_TABLES).
>>>> + */
>>>> +static int walk_mem_range(unsigned long start_pfn, unsigned long
>>>> nr_pages)
>>>> +{
>>>> + struct resource res;
>>>> + unsigned long pfn, end_pfn;
>>>> + u64 orig_end;
>>>> + int ret = -1;
>>>> +
>>>> + res.start = (u64) start_pfn << PAGE_SHIFT;
>>>> + res.end = ((u64)(start_pfn + nr_pages) << PAGE_SHIFT) - 1;
>>>> + res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>>>> + orig_end = res.end;
>>>> + while ((res.start < res.end) &&
>>>> + (find_next_iomem_res(&res, IORES_DESC_NONE, true) >= 0)) {
>>>> + pfn = (res.start + PAGE_SIZE - 1) >> PAGE_SHIFT;
>>>> + end_pfn = (res.end + 1) >> PAGE_SHIFT;
>>>> + if (end_pfn > pfn)
>>>> + ret = (res.desc != IORES_DESC_NONE) ? 1 : 0;
>>>> + if (ret)
>>>> + break;
>>>> + res.start = res.end + 1;
>>>> + res.end = orig_end;
>>>> + }
>>>> + return ret;
>>>> +}
>>>
>>> So the relevant difference between this one and walk_system_ram_range()
>>> is this:
>>>
>>> - ret = (*func)(pfn, end_pfn - pfn, arg);
>>> + ret = (res.desc != IORES_DESC_NONE) ? 1 : 0;
>>>
>>> so it seems to me you can have your own *func() pointer which does that
>>> IORES_DESC_NONE comparison. And then you can define your own workhorse
>>> __walk_memory_range() which gets called by both walk_mem_range() and
>>> walk_system_ram_range() instead of almost duplicating them.
>>>
>>> And looking at walk_system_ram_res(), that one looks similar too except
>>> the pfn computation. But AFAICT the pfn/end_pfn things are computed from
>>> res.start and res.end so it looks to me like all those three functions
>>> are crying for unification...
>>
>> I'll take a look at what it takes to consolidate these with a pre-patch.
>> Then I'll add the new support.
>
> It looks pretty straight forward to combine walk_iomem_res_desc() and
> walk_system_ram_res(). The walk_system_ram_range() function would fit
> easily into this, also, except for the fact that the callback function
> takes unsigned longs vs the u64s of the other functions. Is it worth
> modifying all of the callers of walk_system_ram_range() (which are only
> about 8 locations) to change the callback functions to accept u64s in
> order to consolidate the walk_system_ram_range() function, too?
The more I dig, the more I find that the changes keep expanding. I'll
leave walk_system_ram_range() out of the consolidation for now.
Thanks,
Tom
>
> Thanks,
> Tom
>
>>
>> Thanks,
>> Tom
>>
>>>
^ permalink raw reply
* [PATCH 2/4] crypto: s5p-sss - Remove unused variant field from state container
From: Krzysztof Kozlowski @ 2017-03-17 14:49 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, linux-crypto, linux-kernel,
Vladimir Zapolskiy
Cc: Nathan Royce, Krzysztof Kozlowski
In-Reply-To: <20170317144922.27379-1-krzk@kernel.org>
The driver uses type of device (variant) only during probe so there is
no need to store it for later.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
drivers/crypto/s5p-sss.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 6c620487e9c2..35ea84b7d775 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -190,8 +190,6 @@ struct s5p_aes_dev {
struct crypto_queue queue;
bool busy;
spinlock_t lock;
-
- struct samsung_aes_variant *variant;
};
static struct s5p_aes_dev *s5p_dev;
@@ -852,7 +850,6 @@ static int s5p_aes_probe(struct platform_device *pdev)
}
pdata->busy = false;
- pdata->variant = variant;
pdata->dev = dev;
platform_set_drvdata(pdev, pdata);
s5p_dev = pdata;
--
2.9.3
^ permalink raw reply related
* Re: [ANNOUNCE] /dev/random - a new approach (code for 4.11-rc1)
From: Jason A. Donenfeld @ 2017-03-17 15:31 UTC (permalink / raw)
To: Stephan Müller; +Cc: LKML, Linux Crypto Mailing List
In-Reply-To: <2785457.pDyvZpZC2q@positron.chronox.de>
Hey Stephan,
Have you considered submitting this without so many options? For
example -- just unconditionally using ChaCha20 instead of the
configurable crypto API functions? And either removing the FIPS140
compliance code, and either unconditionally including it, or just
getting rid of it? And finally just making this a part of the kernel
directly, instead of adding this as a standalone optional component?
Jason
^ permalink raw reply
* Re: [PATCH 4/4] crypto: s5p-sss - Use mutex instead of spinlock
From: Bartlomiej Zolnierkiewicz @ 2017-03-17 17:28 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Herbert Xu, David S. Miller, linux-crypto, linux-kernel,
Vladimir Zapolskiy, Nathan Royce
In-Reply-To: <20170317144922.27379-5-krzk@kernel.org>
Hi,
On Friday, March 17, 2017 04:49:22 PM Krzysztof Kozlowski wrote:
> Driver uses threaded interrupt handler so there is no real need for
> using spinlocks for synchronization. Mutexes would do fine and are
> friendlier for overall system preemptivness and real-time behavior.
Are you sure that this conversion is safe? This driver also uses
a tasklet and tasklets run in the interrupt context.
> @@ -667,18 +666,17 @@ static void s5p_tasklet_cb(unsigned long data)
> struct s5p_aes_dev *dev = (struct s5p_aes_dev *)data;
> struct crypto_async_request *async_req, *backlog;
> struct s5p_aes_reqctx *reqctx;
> - unsigned long flags;
>
> - spin_lock_irqsave(&dev->lock, flags);
> + mutex_lock(&dev->lock);
> backlog = crypto_get_backlog(&dev->queue);
> async_req = crypto_dequeue_request(&dev->queue);
>
> if (!async_req) {
> dev->busy = false;
> - spin_unlock_irqrestore(&dev->lock, flags);
> + mutex_unlock(&dev->lock);
> return;
> }
> - spin_unlock_irqrestore(&dev->lock, flags);
> + mutex_unlock(&dev->lock);
Best regards,
^ permalink raw reply
* Re: [PATCH 4/4] crypto: s5p-sss - Use mutex instead of spinlock
From: Krzysztof Kozlowski @ 2017-03-17 17:54 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Herbert Xu, David S. Miller, linux-crypto, linux-kernel,
Vladimir Zapolskiy, Nathan Royce
In-Reply-To: <4289522.7Nbtc4pSSm@amdc3058>
On Fri, Mar 17, 2017 at 06:28:29PM +0100, Bartlomiej Zolnierkiewicz wrote:
>
> Hi,
>
> On Friday, March 17, 2017 04:49:22 PM Krzysztof Kozlowski wrote:
> > Driver uses threaded interrupt handler so there is no real need for
> > using spinlocks for synchronization. Mutexes would do fine and are
> > friendlier for overall system preemptivness and real-time behavior.
>
> Are you sure that this conversion is safe? This driver also uses
> a tasklet and tasklets run in the interrupt context.
>
Yes, you're right. This is not safe and patch should be dropped. Thanks
for spotting this.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/4] crypto: s5p-sss - Close possible race for completed requests
From: Bartlomiej Zolnierkiewicz @ 2017-03-17 17:57 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Herbert Xu, David S. Miller, linux-crypto, linux-kernel,
Vladimir Zapolskiy, Nathan Royce, # v4 . 10 . x
In-Reply-To: <20170317144922.27379-2-krzk@kernel.org>
On Friday, March 17, 2017 04:49:19 PM Krzysztof Kozlowski wrote:
> Driver is capable of handling only one request at a time and it stores
> it in its state container struct s5p_aes_dev. This stored request must be
> protected between concurrent invocations (e.g. completing current
> request and scheduling new one). Combination of lock and "busy" field
> is used for that purpose.
>
> When "busy" field is true, the driver will not accept new request thus
> it will not overwrite currently handled data.
>
> However commit 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion
> on LRW(AES)") moved some of the write to "busy" field out of a lock
> protected critical section. This might lead to potential race between
> completing current request and scheduling a new one. Effectively the
> request completion might try to operate on new crypto request.
>
> Cc: <stable@vger.kernel.org> # v4.10.x
> Fixes: 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion on LRW(AES)")
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Best regards,
^ permalink raw reply
* Re: [PATCH 2/4] crypto: s5p-sss - Remove unused variant field from state container
From: Bartlomiej Zolnierkiewicz @ 2017-03-17 17:57 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Herbert Xu, David S. Miller, linux-crypto, linux-kernel,
Vladimir Zapolskiy, Nathan Royce
In-Reply-To: <20170317144922.27379-3-krzk@kernel.org>
On Friday, March 17, 2017 04:49:20 PM Krzysztof Kozlowski wrote:
> The driver uses type of device (variant) only during probe so there is
> no need to store it for later.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 3/4] crypto: s5p-sss - Document the struct s5p_aes_dev
From: Bartlomiej Zolnierkiewicz @ 2017-03-17 17:57 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Herbert Xu, David S. Miller, linux-crypto, linux-kernel,
Vladimir Zapolskiy, Nathan Royce
In-Reply-To: <20170317144922.27379-4-krzk@kernel.org>
On Friday, March 17, 2017 04:49:21 PM Krzysztof Kozlowski wrote:
> Add kernel-doc to s5p_aes_dev structure.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Best regards,
^ permalink raw reply
* Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
From: Borislav Petkov @ 2017-03-17 18:03 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Peter Zijlstra, Michael Davidson, Michal Marek, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, Herbert Xu, David S. Miller,
Shaohua Li, Dmitry Vyukov, Matthias Kaehlcke, x86, linux-kbuild,
LKML, linux-crypto, linux-raid
In-Reply-To: <CAG_fn=WooTLkzex3hqPgO5pr7GsK3v7Z5riSA7K+-_S=S7rmBQ@mail.gmail.com>
On Fri, Mar 17, 2017 at 01:32:00PM +0100, Alexander Potapenko wrote:
> > IIUC there's only a handful of VLAIS instances in LLVM code, why not
> Sorry, "kernel code", not "LLVM code".
> > just drop them for the sake of better code portability?
And what happens if someone else adds a variable thing like this
somewhere else, builds with gcc, everything's fine and patch gets
applied? Or something else llvm can't stomach.
Does that mean there'll be the occasional, every-so-often whack-a-mole
patchset from someone, fixing the kernel build with llvm yet again?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
From: Dmitry Vyukov @ 2017-03-17 18:47 UTC (permalink / raw)
To: Borislav Petkov
Cc: Alexander Potapenko, Peter Zijlstra, Michael Davidson,
Michal Marek, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Herbert Xu, David S. Miller, Shaohua Li, Matthias Kaehlcke,
x86@kernel.org, open list:KERNEL BUILD + fi..., LKML,
linux-crypto, linux-raid, kbuild-all
In-Reply-To: <20170317180350.63jjysejk2i6vkon@pd.tnic>
On Fri, Mar 17, 2017 at 7:03 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Mar 17, 2017 at 01:32:00PM +0100, Alexander Potapenko wrote:
>> > IIUC there's only a handful of VLAIS instances in LLVM code, why not
>> Sorry, "kernel code", not "LLVM code".
>> > just drop them for the sake of better code portability?
>
> And what happens if someone else adds a variable thing like this
> somewhere else, builds with gcc, everything's fine and patch gets
> applied? Or something else llvm can't stomach.
>
> Does that mean there'll be the occasional, every-so-often whack-a-mole
> patchset from someone, fixing the kernel build with llvm yet again?
This problem is more general and is not specific to clang. It equally
applies to different versions of gcc, different arches and different
configs (namely, anything else than what a developer used for
testing). A known, reasonably well working solution to this problem is
a system of try bots that test patches before commit with different
compilers/configs/archs. We already have such system in the form of
0-day bots. It would be useful to extend it with clang as soon as
kernel builds.
^ permalink raw reply
* Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
From: Michael Davidson @ 2017-03-17 18:52 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Alexander Potapenko, Michal Marek, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Herbert Xu, David S. Miller, Shaohua Li,
Dmitry Vyukov, Matthias Kaehlcke, x86, linux-kbuild, LKML,
linux-crypto, linux-raid
In-Reply-To: <20170317124404.mt3jd5q5vyk63q2w@hirez.programming.kicks-ass.net>
On Fri, Mar 17, 2017 at 5:44 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Be that as it may; what you construct above is disgusting. Surely the
> code can be refactored to not look like dog vomit?
>
> Also; its not immediately obvious conf->copies is 'small' and this
> doesn't blow up the stack; I feel that deserves a comment somewhere.
>
I agree that the code is horrible.
It is, in fact, exactly the same solution that was used to remove
variable length arrays in structs from several of the crypto drivers a
few years ago - see the definition of SHASH_DESC_ON_STACK() in
"crypto/hash.h" - I did not, however, hide the horrors in a macro
preferring to leave the implementation visible as a warning to whoever
might touch the code next.
I believe that the actual stack usage is exactly the same as it was previously.
I can certainly wrap this up in a macro and add comments with
appropriately dire warnings in it if you feel that is both necessary
and sufficient.
^ permalink raw reply
* Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
From: Borislav Petkov @ 2017-03-17 18:57 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Alexander Potapenko, Peter Zijlstra, Michael Davidson,
Michal Marek, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Herbert Xu, David S. Miller, Shaohua Li, Matthias Kaehlcke,
x86@kernel.org, open list:KERNEL BUILD + fi..., LKML,
linux-crypto, linux-raid, kbuild-all
In-Reply-To: <CACT4Y+Ysvjz+UzEi0dTtsNxxnG7WXTiZPr+KSk8HHwSu7vX1ew@mail.gmail.com>
On Fri, Mar 17, 2017 at 07:47:33PM +0100, Dmitry Vyukov wrote:
> This problem is more general and is not specific to clang. It equally
> applies to different versions of gcc, different arches and different
> configs (namely, anything else than what a developer used for
> testing).
I guess. We do carry a bunch of gcc workarounds along with the cc-*
macros in scripts/Kbuild.include.
> A known, reasonably well working solution to this problem is
> a system of try bots that test patches before commit with different
> compilers/configs/archs. We already have such system in the form of
> 0-day bots. It would be useful to extend it with clang as soon as
> kernel builds.
Has someone actually already talked to Fengguang about it?
Oh, and the stupid question: why the effort to build the kernel
with clang at all? Just because or are there some actual, palpable
advantages?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
From: Dmitry Vyukov @ 2017-03-17 19:05 UTC (permalink / raw)
To: Borislav Petkov
Cc: Alexander Potapenko, Peter Zijlstra, Michael Davidson,
Michal Marek, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Herbert Xu, David S. Miller, Shaohua Li, Matthias Kaehlcke,
x86@kernel.org, open list:KERNEL BUILD + fi..., LKML,
linux-crypto, linux-raid, kbuild-all, Fengguang Wu
In-Reply-To: <20170317185720.5s7qa6hl233t24ag@pd.tnic>
On Fri, Mar 17, 2017 at 7:57 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Mar 17, 2017 at 07:47:33PM +0100, Dmitry Vyukov wrote:
>> This problem is more general and is not specific to clang. It equally
>> applies to different versions of gcc, different arches and different
>> configs (namely, anything else than what a developer used for
>> testing).
>
> I guess. We do carry a bunch of gcc workarounds along with the cc-*
> macros in scripts/Kbuild.include.
>
>> A known, reasonably well working solution to this problem is
>> a system of try bots that test patches before commit with different
>> compilers/configs/archs. We already have such system in the form of
>> 0-day bots. It would be useful to extend it with clang as soon as
>> kernel builds.
>
> Has someone actually already talked to Fengguang about it?
+Fengguang
> Oh, and the stupid question: why the effort to build the kernel
> with clang at all? Just because or are there some actual, palpable
> advantages?
On our side it is:
- clang make it possible to implement KMSAN (dynamic detection of
uses of uninit memory)
- better code coverage for fuzzing
- why simpler and faster development (e.g. we can port our user-space
hardening technologies -- CFI and SafeStack)
You can also find some reasons in the Why section of LLVM-Linux project:
http://llvm.linuxfoundation.org/index.php/Main_Page
^ permalink raw reply
* Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
From: Peter Zijlstra @ 2017-03-17 19:26 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Borislav Petkov, Alexander Potapenko, Michael Davidson,
Michal Marek, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Herbert Xu, David S. Miller, Shaohua Li, Matthias Kaehlcke,
x86@kernel.org, open list:KERNEL BUILD + fi..., LKML,
linux-crypto, linux-raid, kbuild-all, Fengguang Wu
In-Reply-To: <CACT4Y+bybdpqB71=inx8amwr188Mb2QyBeRTDdWZ3AFyDwVX0A@mail.gmail.com>
On Fri, Mar 17, 2017 at 08:05:16PM +0100, Dmitry Vyukov wrote:
> You can also find some reasons in the Why section of LLVM-Linux project:
> http://llvm.linuxfoundation.org/index.php/Main_Page
>From that:
- LLVM/Clang is a fast moving project with many things fixed quickly
and features added.
So what's the deal with that 5 year old bug you want us to work around?
Also, clang doesn't support asm cc flags output and a few other
extensions last time I checked.
^ permalink raw reply
* Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
From: Peter Zijlstra @ 2017-03-17 19:27 UTC (permalink / raw)
To: Michael Davidson
Cc: Alexander Potapenko, Michal Marek, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Herbert Xu, David S. Miller, Shaohua Li,
Dmitry Vyukov, Matthias Kaehlcke, x86, linux-kbuild, LKML,
linux-crypto, linux-raid
In-Reply-To: <CA+=D-XXy9yEQc-jLxSqRUwq=4ADnFqaxnsjSzV4=8jOisym=bQ@mail.gmail.com>
On Fri, Mar 17, 2017 at 11:52:01AM -0700, Michael Davidson wrote:
> On Fri, Mar 17, 2017 at 5:44 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > Be that as it may; what you construct above is disgusting. Surely the
> > code can be refactored to not look like dog vomit?
> >
> > Also; its not immediately obvious conf->copies is 'small' and this
> > doesn't blow up the stack; I feel that deserves a comment somewhere.
> >
>
> I agree that the code is horrible.
>
> It is, in fact, exactly the same solution that was used to remove
> variable length arrays in structs from several of the crypto drivers a
> few years ago - see the definition of SHASH_DESC_ON_STACK() in
> "crypto/hash.h" - I did not, however, hide the horrors in a macro
> preferring to leave the implementation visible as a warning to whoever
> might touch the code next.
>
> I believe that the actual stack usage is exactly the same as it was previously.
>
> I can certainly wrap this up in a macro and add comments with
> appropriately dire warnings in it if you feel that is both necessary
> and sufficient.
We got away with ugly in the past, so we should get to do it again?
^ permalink raw reply
* Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
From: Peter Zijlstra @ 2017-03-17 19:29 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Borislav Petkov, Alexander Potapenko, Michael Davidson,
Michal Marek, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Herbert Xu, David S. Miller, Shaohua Li, Matthias Kaehlcke,
x86@kernel.org, open list:KERNEL BUILD + fi..., LKML,
linux-crypto, linux-raid, kbuild-all, Fengguang Wu
In-Reply-To: <20170317192642.qnrf7xuopxzapl2r@hirez.programming.kicks-ass.net>
On Fri, Mar 17, 2017 at 08:26:42PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 17, 2017 at 08:05:16PM +0100, Dmitry Vyukov wrote:
> > You can also find some reasons in the Why section of LLVM-Linux project:
> > http://llvm.linuxfoundation.org/index.php/Main_Page
>
> From that:
>
> - LLVM/Clang is a fast moving project with many things fixed quickly
> and features added.
>
> So what's the deal with that 5 year old bug you want us to work around?
>
> Also, clang doesn't support asm cc flags output and a few other
> extensions last time I checked.
>
Another great one:
- BSD License (some people prefer this license to the GPL)
Seems a very weak argument to make when talking about the Linux Kernel
which is very explicitly GPLv2 (and not later).
^ permalink raw reply
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