* Re: [patch RFC 00/15] mm/highmem: Provide a preemptible variant of kmap_atomic & friends
From: Thomas Gleixner @ 2020-09-23 12:33 UTC (permalink / raw)
To: peterz
Cc: Juri Lelli, David Airlie, Sebastian Andrzej Siewior,
Joonas Lahtinen, dri-devel, linux-mips, Ben Segall, Max Filippov,
Guo Ren, linux-sparc, Vincent Chen, Will Deacon, Ard Biesheuvel,
linux-arch, Vincent Guittot, Herbert Xu, the arch/x86 maintainers,
Russell King, linux-csky, Mel Gorman,
open list:SYNOPSYS ARC ARCHITECTURE, linux-xtensa, Paul McKenney,
intel-gfx, linuxppc-dev, Steven Rostedt, Jani Nikula,
Rodrigo Vivi, Dietmar Eggemann, Linux ARM, Chris Zankel,
Michal Simek, Thomas Bogendoerfer, Nick Hu, Linux-MM,
Linus Torvalds, LKML, Arnd Bergmann, Daniel Vetter, Vineet Gupta,
Paul Mackerras, Andrew Morton, Daniel Bristot de Oliveira,
David S. Miller, Greentime Hu
In-Reply-To: <20200923101953.GT2674@hirez.programming.kicks-ass.net>
On Wed, Sep 23 2020 at 12:19, peterz wrote:
> On Mon, Sep 21, 2020 at 09:27:57PM +0200, Thomas Gleixner wrote:
>> Alternatively this could of course be solved with per CPU page tables
>> which will come around some day anyway I fear.
>
> Previously (with PTI) we looked at making the entire kernel map per-CPU,
> and that takes a 2K copy on switch_mm() (or more general, the user part
> of whatever the top level directory is for architectures that have a
> shared kernel/user page-table setup in the first place).
>
> The idea was having a fixed per-cpu kernel page-table, share a bunch of
> (kernel) page-tables between all CPUs and then copy in the user part on
> switch.
>
> I've forgotten what the plan was for ASID/PCID in that scheme.
>
> For x86_64 we've been fearing the performance of that 2k copy, but I
> don't think we've ever actually bit the bullet and implemented it to see
> how bad it really is.
I actually did at some point and depending on the workload the overhead
was clearly measurable. And yes, it fell apart with PCID and I could not
come up with a scheme for it which did not suck horribly. So I burried
the patches in the poison cabinet.
Aside of that, we'd need to implement that for a eight other
architectures as well...
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH] i2c: cpm: Fix i2c_ram structure
From: Vincent Nicolas @ 2020-09-23 12:55 UTC (permalink / raw)
To: Christophe Leroy, jochen@scram.de
Cc: linuxppc-dev@lists.ozlabs.org, linux-i2c@vger.kernel.org
In-Reply-To: <2ecfe18a-61f6-bb0e-22c5-b7ab79a77d03@csgroup.eu>
> ________________________________________
> From: Christophe Leroy <christophe.leroy@csgroup.eu>
> Sent: Wednesday, 23 September 2020 10:01
> To: Vincent Nicolas; jochen@scram.de
> Cc: linuxppc-dev@lists.ozlabs.org; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH] i2c: cpm: Fix i2c_ram structure
>
>
>
> Le 23/09/2020 à 09:18, Vincent Nicolas a écrit :
>>
>>
>>
>> From: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Sent: Tuesday, 22 September 2020 14:38
>> To: Vincent Nicolas <Nicolas.Vincent@vossloh.com>; jochen@scram.de <jochen@scram.de>
>> Cc: linuxppc-dev@lists.ozlabs.org <linuxppc-dev@lists.ozlabs.org>; linux-i2c@vger.kernel.org <linux-i2c@vger.kernel.org>
>> Subject: Re: [PATCH] i2c: cpm: Fix i2c_ram structure
>>
>>
>>
>> Le 22/09/2020 à 11:04, nico.vince@gmail.com a écrit :
>>> From: Nicolas VINCENT <nicolas.vincent@vossloh.com>
>>>
>>> the i2c_ram structure is missing the sdmatmp field mentionned in
>>> datasheet for MPC8272 at paragraph 36.5. With this field missing, the
>>> hardware would write past the allocated memory done through
>>> cpm_muram_alloc for the i2c_ram structure and land in memory allocated
>>> for the buffers descriptors corrupting the cbd_bufaddr field. Since this
>>> field is only set during setup(), the first i2c transaction would work
>>> and the following would send data read from an arbitrary memory
>>> location.
>>>
>>> Signed-off-by: Nicolas VINCENT <nicolas.vincent@vossloh.com>
>>> ---
>>> drivers/i2c/busses/i2c-cpm.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
>>> index 1213e1932ccb..c5700addbf65 100644
>>> --- a/drivers/i2c/busses/i2c-cpm.c
>>> +++ b/drivers/i2c/busses/i2c-cpm.c
>>> @@ -64,7 +64,8 @@ struct i2c_ram {
>>> uint txtmp; /* Internal */
>>> char res1[4]; /* Reserved */
>>> ushort rpbase; /* Relocation pointer */
>>> - char res2[2]; /* Reserved */
>>> + char res2[6]; /* Reserved */
>>> + uint sdmatmp; /* Internal */
>>
>> On CPM1, I2C param RAM has size 0x30 (offset 0x1c80-0x1caf)
>>
>> Your change overlaps the miscellaneous area that contains CP Microcode
>> Revision Number, ref MPC885 Reference Manual §18.7.3
>>
>> As far as I understand the mpc885 contains in the dts the compatible=fsl,cpm1-i2c which is used in cpm-i2c.c to either determine the address of the i2c_ram structure (cpm1), or dynamically allocate it with cpm_muram_alloc (cpm2).
>> In the first case the structure will indeed overlaps with the miscellaneous section but since the sdmatmp is only used by cpm2 hardware it shall not be an issue.
>>
>> Please, let me know if I am mistaken. If the patch cannot be accepted as is, I would gladly accept pointers on how to address this kind of issue.
>
>
> Please use a mail client that properly sets the > in front of
> original/answered text. Here your mailer has mixed you text and mine,
> that's unusable on the long term.
I changed my configuration, please let me know if there are still issues
>
>
> I think you are right on the fact that it doesn't seem to be an issue.
> Nevertheless, that's confusing.
>
> What I would suggest is to leave res2[2] as is, and add something like:
>
> /* The following elements are only for CPM2 */
> char res3[4]; /* Reserved */
> uint sdmatmp; /* Internal */
I'll repost the patch like this then.
>
>
> Other solution (not sure that's the best solution thought) would be to
> do as in spi-fsl-cpm: use iic_t structure from asm/cpm1.h when
> CONFIG_CPM1 is selected and use iic_t from asm/cpm2.h when CONFIG_CPM2
> is selected, taking into account that CONFIG_CPM1 and CONFIG_CPM2 are
> mutually exclusive at the time being.
Unless someone argues for this solution I will go with the first one.
Thank again for your time and quick responses.
Nicolas.
^ permalink raw reply
* Re: [PATCH] i2c: cpm: Fix i2c_ram structure
From: Christophe Leroy @ 2020-09-23 13:04 UTC (permalink / raw)
To: Vincent Nicolas, jochen@scram.de
Cc: linuxppc-dev@lists.ozlabs.org, linux-i2c@vger.kernel.org
In-Reply-To: <PR3P193MB0731A954E01511C22259ECF0F1380@PR3P193MB0731.EURP193.PROD.OUTLOOK.COM>
Le 23/09/2020 à 14:55, Vincent Nicolas a écrit :
>
>
>> ________________________________________
>> From: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Sent: Wednesday, 23 September 2020 10:01
>> To: Vincent Nicolas; jochen@scram.de
>> Cc: linuxppc-dev@lists.ozlabs.org; linux-i2c@vger.kernel.org
>> Subject: Re: [PATCH] i2c: cpm: Fix i2c_ram structure
>>
>>
>>
>> Le 23/09/2020 à 09:18, Vincent Nicolas a écrit :
>>>
>>>
>>>
>>> From: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> Sent: Tuesday, 22 September 2020 14:38
>>> To: Vincent Nicolas <Nicolas.Vincent@vossloh.com>; jochen@scram.de <jochen@scram.de>
>>> Cc: linuxppc-dev@lists.ozlabs.org <linuxppc-dev@lists.ozlabs.org>; linux-i2c@vger.kernel.org <linux-i2c@vger.kernel.org>
>>> Subject: Re: [PATCH] i2c: cpm: Fix i2c_ram structure
>>>
>>>
>>>
>>> Le 22/09/2020 à 11:04, nico.vince@gmail.com a écrit :
>>>> From: Nicolas VINCENT <nicolas.vincent@vossloh.com>
>>>>
>>>> the i2c_ram structure is missing the sdmatmp field mentionned in
>>>> datasheet for MPC8272 at paragraph 36.5. With this field missing, the
>>>> hardware would write past the allocated memory done through
>>>> cpm_muram_alloc for the i2c_ram structure and land in memory allocated
>>>> for the buffers descriptors corrupting the cbd_bufaddr field. Since this
>>>> field is only set during setup(), the first i2c transaction would work
>>>> and the following would send data read from an arbitrary memory
>>>> location.
>>>>
>>>> Signed-off-by: Nicolas VINCENT <nicolas.vincent@vossloh.com>
>>>> ---
>>>> drivers/i2c/busses/i2c-cpm.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
>>>> index 1213e1932ccb..c5700addbf65 100644
>>>> --- a/drivers/i2c/busses/i2c-cpm.c
>>>> +++ b/drivers/i2c/busses/i2c-cpm.c
>>>> @@ -64,7 +64,8 @@ struct i2c_ram {
>>>> uint txtmp; /* Internal */
>>>> char res1[4]; /* Reserved */
>>>> ushort rpbase; /* Relocation pointer */
>>>> - char res2[2]; /* Reserved */
>>>> + char res2[6]; /* Reserved */
>>>> + uint sdmatmp; /* Internal */
>>>
>>> On CPM1, I2C param RAM has size 0x30 (offset 0x1c80-0x1caf)
>>>
>>> Your change overlaps the miscellaneous area that contains CP Microcode
>>> Revision Number, ref MPC885 Reference Manual §18.7.3
>>>
>>> As far as I understand the mpc885 contains in the dts the compatible=fsl,cpm1-i2c which is used in cpm-i2c.c to either determine the address of the i2c_ram structure (cpm1), or dynamically allocate it with cpm_muram_alloc (cpm2).
>>> In the first case the structure will indeed overlaps with the miscellaneous section but since the sdmatmp is only used by cpm2 hardware it shall not be an issue.
>>>
>>> Please, let me know if I am mistaken. If the patch cannot be accepted as is, I would gladly accept pointers on how to address this kind of issue.
>>
>>
>> Please use a mail client that properly sets the > in front of
>> original/answered text. Here your mailer has mixed you text and mine,
>> that's unusable on the long term.
>
> I changed my configuration, please let me know if there are still issues
Yes it is OK now.
Christophe
^ permalink raw reply
* Re: [PATCH 1/9] kernel: add a PF_FORCE_COMPAT flag
From: Al Viro @ 2020-09-23 13:22 UTC (permalink / raw)
To: Pavel Begunkov
Cc: linux-aio, open list:MIPS, David Howells, Linux-MM, keyrings,
sparclinux, Christoph Hellwig, linux-arch, linux-s390,
Linux SCSI List, X86 ML, Arnd Bergmann, linux-block,
Andy Lutomirski, io-uring, linux-arm-kernel, Jens Axboe,
Parisc List, Network Development, LKML, LSM List, Linux FS Devel,
Andrew Morton, linuxppc-dev
In-Reply-To: <91209170-dcb4-d9ee-afa0-a819f8877b86@gmail.com>
On Wed, Sep 23, 2020 at 11:01:34AM +0300, Pavel Begunkov wrote:
> > I'm not following why that would be considered a valid option,
> > as that clearly breaks existing users that update from a 32-bit
> > kernel to a 64-bit one.
>
> Do you mean users who move 32-bit binaries (without recompiling) to a
> new x64 kernel? Does the kernel guarantees that to work?
Yes.
No further (printable) comments for now...
^ permalink raw reply
* Re: [patch RFC 00/15] mm/highmem: Provide a preemptible variant of kmap_atomic & friends
From: Thomas Gleixner @ 2020-09-23 13:35 UTC (permalink / raw)
To: peterz
Cc: Juri Lelli, David Airlie, Sebastian Andrzej Siewior,
Joonas Lahtinen, dri-devel, linux-mips, Ben Segall, Max Filippov,
Guo Ren, linux-sparc, Vincent Chen, Will Deacon, Ard Biesheuvel,
linux-arch, Vincent Guittot, Herbert Xu, the arch/x86 maintainers,
Russell King, linux-csky, Mel Gorman,
open list:SYNOPSYS ARC ARCHITECTURE, linux-xtensa, Paul McKenney,
intel-gfx, linuxppc-dev, Steven Rostedt, Jani Nikula,
Rodrigo Vivi, Dietmar Eggemann, Linux ARM, Chris Zankel,
Michal Simek, Thomas Bogendoerfer, Nick Hu, Linux-MM,
Linus Torvalds, LKML, Arnd Bergmann, Daniel Vetter, Vineet Gupta,
Paul Mackerras, Andrew Morton, Daniel Bristot de Oliveira,
David S. Miller, Greentime Hu
In-Reply-To: <20200923084032.GU1362448@hirez.programming.kicks-ass.net>
On Wed, Sep 23 2020 at 10:40, peterz wrote:
> Right, so I'm concerned. migrate_disable() wrecks pretty much all
> Real-Time scheduler theory we have, and PREEMPRT_RT bringing it in is
> somewhat ironic.
It's even more ironic that the approach of PREEMPT_RT has been
'pragmatic ignorance of theory' from the very beginning and despite
violating all theories it still works. :)
> Yes, it allows breaking up non-preemptible regions of non-deterministic
> duration, and thereby both reduce and bound the scheduling latency, the
> cost for doing that is that the theory on CPU utilization/bandwidth go
> out the window.
I agree, that the theory goes out of the window, but does it matter in
practice? I've yet to see a report of migrate disable stacking being the
culprit of a missed deadline and I surely have stared at lots of reports
in the past 10+ years.
> To easily see this consider an SMP system with a number of tasks equal
> to the number of CPUs. On a regular (preempt_disable) kernel we can
> always run each task, by virtue of always having an idle CPU to take the
> task.
>
> However, with migrate_disable() we can have each task preempted in a
> migrate_disable() region, worse we can stack them all on the _same_ CPU
> (super ridiculous odds, sure). And then we end up only able to run one
> task, with the rest of the CPUs picking their nose.
>
> The end result is that, like with unbounded latency, we will still miss
> our deadline, simply because we got starved for CPU.
I'm well aware of that.
> Now, while we could (with a _lot_ of work) rework the kernel to not rely
> on the implicit per-cpu ness of things like spinlock_t, the moment we
> bring in basic primitives that rely on migrate_disable() we're stuck
> with it.
Right, but we are stuck with per CPUness and distangling that is just
infeasible IMO.
> The problem is; afaict there's been no research into this problem.
There is no research on a lot of things the kernel does :)
> There might be scheduling (read: balancing) schemes that can
> mitigate/solve this problem, or it might prove to be a 'hard' problem,
> I just don't know.
In practive balancing surely can take the number of preempted tasks
which are in a migrate disable section into account which would be just
another measure to work around the fact that the kernel is not adhering
to the theories. It never did that even w/o migrate disable.
> But once we start down this road, it's going to be hell to get rid of
> it.
Like most of the other things the kernel came up with to deal with the
oddities of modern hardware :)
> That's why I've been arguing (for many years) to strictly limit this to
> PREEMPT_RT and only as a gap-stop, not a fundamental primitive to build
> on.
I know, but short of rewriting the world, I'm not seing the faintest
plan to remove the stop gap. :)
As we discussed not long ago we have too many inconsistent preemption
models already. RT is adding yet another one. And that's worse than
introducing migrate disable as a general available facility.
IMO, reaching a point of consistency where our different preemption
models (NONE, VOLUNTARY, PREEMPT. RT) build on each other is far more
important.
For !RT migrate disable is far less of an danger than for RT kernels
because the amount of code which will use it is rather limited compared
to code which still will disable preemption implicit through spin and rw
locks.
On RT converting these locks to 'sleepable spinlocks' is just possible
because RT forces almost everything into task context and migrate
disable is just the obvious decomposition of preempt disable which
implicitely disables migration.
But that means that RT is by orders of magnitude more prone to run into
the scheduling trainwreck you are worried about. It just refuses to do
so at least with real world work loads.
I'm surely in favour of having solid theories behind implementation, but
at some point you just have to bite the bullet and chose pragmatism in
order to make progress.
Proliferating inconsistency is not real progress, as it is violating the
most fundamental engineering principles. That's by far more dangerous
than violating scheduling theories which are built on perfect models and
therefore enforce violation by practical implementations anyway.
Thanks,
tglx
^ permalink raw reply
* [PATCH v2] i2c: cpm: Fix i2c_ram structure
From: nico.vince @ 2020-09-23 14:08 UTC (permalink / raw)
To: jochen; +Cc: Nicolas VINCENT, linuxppc-dev, linux-i2c
From: Nicolas VINCENT <nicolas.vincent@vossloh.com>
the i2c_ram structure is missing the sdmatmp field mentionned in
datasheet for MPC8272 at paragraph 36.5. With this field missing, the
hardware would write past the allocated memory done through
cpm_muram_alloc for the i2c_ram structure and land in memory allocated
for the buffers descriptors corrupting the cbd_bufaddr field. Since this
field is only set during setup(), the first i2c transaction would work
and the following would send data read from an arbitrary memory
location.
Signed-off-by: Nicolas VINCENT <nicolas.vincent@vossloh.com>
---
drivers/i2c/busses/i2c-cpm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
index 1213e1932ccb..24d584a1c9a7 100644
--- a/drivers/i2c/busses/i2c-cpm.c
+++ b/drivers/i2c/busses/i2c-cpm.c
@@ -65,6 +65,9 @@ struct i2c_ram {
char res1[4]; /* Reserved */
ushort rpbase; /* Relocation pointer */
char res2[2]; /* Reserved */
+ /* The following elements are only for CPM2 */
+ char res3[4]; /* Reserved */
+ uint sdmatmp; /* Internal */
};
#define I2COM_START 0x80
--
2.17.1
^ permalink raw reply related
* Re: [PATCH kernel] powerpc/dma: Fix dma_map_ops::get_required_mask
From: Christoph Hellwig @ 2020-09-23 14:10 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Oliver OHalloran, linuxppc-dev@lists.ozlabs.org,
Christoph Hellwig, Cédric Le Goater
In-Reply-To: <93424419-3476-fc07-8a83-8d9d39062810@ozlabs.ru>
On Tue, Sep 22, 2020 at 12:26:18PM +1000, Alexey Kardashevskiy wrote:
> > Well, the original intent of dma_get_required_mask is to return the
> > mask that the driver then uses to figure out what to set, so what aacraid
> > does fits that use case.
>
> What was the original intent exactly? The driver asks for the minimum or
> maximum DMA mask the platform supports?
>
> As for now, we (ppc64/powernv) can do:
> 1. bypass (==64bit)
> 2. a DMA window which used to be limited by 2GB but not anymore.
>
> I can understand if the driver asked for required mask in expectation to
> receive "less or equal than 32bit" and "more than 32 bit" and choose.
> And this probably was the intent as at the time when the bug was
> introduced, the window was always smaller than 4GB.
>
> But today the window is bigger than than (44 bits now, or a similar
> value, depends on max page order) so the returned mask is >32. Which
> still enables that DAC in aacraid but I suspect this is accidental.
I think for powernv returning 64-bit always would make a lot of sense.
AFAIK all of powernv is PCIe and not legacy PCI, so returning anything
less isn't going to help to optimize anything.
> > Of course that idea is pretty bogus for
> > PCIe devices.
>
> Why? From the PHB side, there are windows. From the device side, there
> are many crippled devices, like, no GPU I saw in last years supported
> more than 48bit.
Yes, but dma_get_required_mask is misnamed - the mask is not required,
it is the optimal mask. Even if the window is smaller we handle it
some way, usually by using swiotlb, or by iommu tricks in your case.
> > I suspect the right fix is to just not query dma_get_required_mask for
> > PCIe devices in aacraid (and other drivers that do something similar).
>
> May be, if you write nice and big comment next to
> dma_get_required_mask() explaining exactly what it does, then I will
> realize I am getting this all wrong and we will move to fixing the
> drivers :)
Yes, it needs a comment or two, and probaby be renamed to
dma_get_optimal_dma_mask, and a cleanup of most users. I've added it
to my ever growing TODO list, but I would not be unhappy if someone
else gives it a spin.
^ permalink raw reply
* Re: [PATCH v2] i2c: cpm: Fix i2c_ram structure
From: Jochen Friedrich @ 2020-09-23 14:12 UTC (permalink / raw)
To: nicolas.vincent; +Cc: linuxppc-dev, linux-i2c
In-Reply-To: <20200923140840.8700-1-nicolas.vincent@vossloh.com>
Acked-by: Jochen Friedrich <jochen@scram.de>
Am 23.09.2020 um 16:08 schrieb nico.vince@gmail.com:
> From: Nicolas VINCENT <nicolas.vincent@vossloh.com>
>
> the i2c_ram structure is missing the sdmatmp field mentionned in
> datasheet for MPC8272 at paragraph 36.5. With this field missing, the
> hardware would write past the allocated memory done through
> cpm_muram_alloc for the i2c_ram structure and land in memory allocated
> for the buffers descriptors corrupting the cbd_bufaddr field. Since this
> field is only set during setup(), the first i2c transaction would work
> and the following would send data read from an arbitrary memory
> location.
>
> Signed-off-by: Nicolas VINCENT <nicolas.vincent@vossloh.com>
> ---
> drivers/i2c/busses/i2c-cpm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
> index 1213e1932ccb..24d584a1c9a7 100644
> --- a/drivers/i2c/busses/i2c-cpm.c
> +++ b/drivers/i2c/busses/i2c-cpm.c
> @@ -65,6 +65,9 @@ struct i2c_ram {
> char res1[4]; /* Reserved */
> ushort rpbase; /* Relocation pointer */
> char res2[2]; /* Reserved */
> + /* The following elements are only for CPM2 */
> + char res3[4]; /* Reserved */
> + uint sdmatmp; /* Internal */
> };
>
> #define I2COM_START 0x80
>
^ permalink raw reply
* Re: [PATCH 3/9] iov_iter: refactor rw_copy_check_uvector and import_iovec
From: Al Viro @ 2020-09-23 14:16 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-aio, linux-mips, David Howells, linux-mm, keyrings,
sparclinux, linux-arch, linux-s390, linux-scsi, Linus Torvalds,
Arnd Bergmann, linux-block, io-uring, linux-arm-kernel,
Jens Axboe, linux-parisc, netdev, linux-kernel,
linux-security-module, David Laight, linux-fsdevel, Andrew Morton,
linuxppc-dev
In-Reply-To: <20200923060547.16903-4-hch@lst.de>
On Wed, Sep 23, 2020 at 08:05:41AM +0200, Christoph Hellwig wrote:
> +struct iovec *iovec_from_user(const struct iovec __user *uvec,
> + unsigned long nr_segs, unsigned long fast_segs,
Hmm... For fast_segs unsigned long had always been ridiculous
(4G struct iovec on caller stack frame?), but that got me wondering about
nr_segs and I wish I'd thought of that when introducing import_iovec().
The thing is, import_iovec() takes unsigned int there. Which is fine
(hell, the maximal value that can be accepted in 1024), except that
we do pass unsigned long syscall argument to it in some places.
E.g. vfs_readv() quietly truncates vlen to 32 bits, and vlen can
come unchanged through sys_readv() -> do_readv() -> vfs_readv().
With unsigned long passed by syscall glue.
AFAICS, passing 4G+1 as the third argument to readv(2) on 64bit box
will be quietly treated as 1 these days. Which would be fine, except
that before "switch {compat_,}do_readv_writev() to {compat_,}import_iovec()"
it used to fail with -EINVAL.
Userland, BTW, describes readv(2) iovcnt as int; process_vm_readv(),
OTOH, has these counts unsigned long from the userland POV...
I suppose we ought to switch import_iovec() to unsigned long for nr_segs ;-/
Strictly speaking that had been a userland ABI change, even though nothing
except regression tests checking for expected errors would've been likely
to notice. And it looks like no regression tests covered that one...
Linus, does that qualify for your "if no userland has noticed the change,
it's not a breakage"?
^ permalink raw reply
* Re: [PATCH 5/9] fs: remove various compat readv/writev helpers
From: Al Viro @ 2020-09-23 14:25 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-aio, linux-mips, David Howells, linux-mm, keyrings,
sparclinux, linux-arch, linux-s390, linux-scsi, Arnd Bergmann,
linux-block, io-uring, linux-arm-kernel, Jens Axboe, linux-parisc,
netdev, linux-kernel, linux-security-module, David Laight,
linux-fsdevel, Andrew Morton, linuxppc-dev
In-Reply-To: <20200923060547.16903-6-hch@lst.de>
On Wed, Sep 23, 2020 at 08:05:43AM +0200, Christoph Hellwig wrote:
> COMPAT_SYSCALL_DEFINE3(readv, compat_ulong_t, fd,
> - const struct compat_iovec __user *,vec,
> + const struct iovec __user *, vec,
Um... Will it even compile?
> #ifdef __ARCH_WANT_COMPAT_SYS_PREADV64
> COMPAT_SYSCALL_DEFINE4(preadv64, unsigned long, fd,
> - const struct compat_iovec __user *,vec,
> + const struct iovec __user *, vec,
Ditto. Look into include/linux/compat.h and you'll see
asmlinkage long compat_sys_preadv64(unsigned long fd,
const struct compat_iovec __user *vec,
unsigned long vlen, loff_t pos);
How does that manage to avoid the compiler screaming bloody
murder?
^ permalink raw reply
* Re: [PATCH 5/9] fs: remove various compat readv/writev helpers
From: Christoph Hellwig @ 2020-09-23 14:32 UTC (permalink / raw)
To: Al Viro
Cc: linux-aio, linux-mips, David Howells, linux-mm, keyrings,
sparclinux, Christoph Hellwig, linux-arch, linux-s390, linux-scsi,
Arnd Bergmann, linux-block, io-uring, linux-arm-kernel,
Jens Axboe, linux-parisc, netdev, linux-kernel,
linux-security-module, David Laight, linux-fsdevel, Andrew Morton,
linuxppc-dev
In-Reply-To: <20200923142549.GK3421308@ZenIV.linux.org.uk>
On Wed, Sep 23, 2020 at 03:25:49PM +0100, Al Viro wrote:
> On Wed, Sep 23, 2020 at 08:05:43AM +0200, Christoph Hellwig wrote:
> > COMPAT_SYSCALL_DEFINE3(readv, compat_ulong_t, fd,
> > - const struct compat_iovec __user *,vec,
> > + const struct iovec __user *, vec,
>
> Um... Will it even compile?
>
> > #ifdef __ARCH_WANT_COMPAT_SYS_PREADV64
> > COMPAT_SYSCALL_DEFINE4(preadv64, unsigned long, fd,
> > - const struct compat_iovec __user *,vec,
> > + const struct iovec __user *, vec,
>
> Ditto. Look into include/linux/compat.h and you'll see
>
> asmlinkage long compat_sys_preadv64(unsigned long fd,
> const struct compat_iovec __user *vec,
> unsigned long vlen, loff_t pos);
>
> How does that manage to avoid the compiler screaming bloody
> murder?
That's a very good question. But it does not just compile but actually
works. Probably because all the syscall wrappers mean that we don't
actually generate the normal names. I just tried this:
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -468,7 +468,7 @@ asmlinkage long sys_lseek(unsigned int fd, off_t offset,
asmlinkage long sys_read(unsigned int fd, char __user *buf, size_t count);
asmlinkage long sys_write(unsigned int fd, const char __user *buf,
size_t count);
-asmlinkage long sys_readv(unsigned long fd,
+asmlinkage long sys_readv(void *fd,
for fun, and the compiler doesn't care either..
^ permalink raw reply
* Re: [patch RFC 00/15] mm/highmem: Provide a preemptible variant of kmap_atomic & friends
From: Thomas Gleixner @ 2020-09-23 14:33 UTC (permalink / raw)
To: Linus Torvalds
Cc: Juri Lelli, Peter Zijlstra, Sebastian Andrzej Siewior,
Joonas Lahtinen, dri-devel, linux-mips, Ben Segall, Max Filippov,
Guo Ren, linux-sparc, Vincent Chen, Will Deacon, Ard Biesheuvel,
linux-arch, Vincent Guittot, Herbert Xu, the arch/x86 maintainers,
Russell King, linux-csky, David Airlie, Mel Gorman,
open list:SYNOPSYS ARC ARCHITECTURE, linux-xtensa, Paul McKenney,
intel-gfx, linuxppc-dev, Steven Rostedt, Jani Nikula,
Rodrigo Vivi, Dietmar Eggemann, Linux ARM, Chris Zankel,
Michal Simek, Thomas Bogendoerfer, Nick Hu, Linux-MM,
Vineet Gupta, LKML, Arnd Bergmann, Daniel Vetter, Paul Mackerras,
Andrew Morton, Daniel Bristot de Oliveira, David S. Miller,
Greentime Hu
In-Reply-To: <87sgbbaq0y.fsf@nanos.tec.linutronix.de>
On Mon, Sep 21 2020 at 21:27, Thomas Gleixner wrote:
> On Mon, Sep 21 2020 at 09:24, Linus Torvalds wrote:
>> On Mon, Sep 21, 2020 at 12:39 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>> Maybe we really *could* call this new kmap functionality something
>> like "kmap_percpu()" (or maybe "local" is good enough), and make it
>> act like your RT code does for spinlocks - not disable preemption, but
>> only disabling CPU migration.
>
> I"m all for it, but the scheduler people have opinions :)
I just took the latest version of migrate disable patches
https://lore.kernel.org/r/20200921163557.234036895@infradead.org
removed the RT dependency on top of them and adopted the kmap stuff
(addressing the various comments while at it and unbreaking ARM).
I'm not going to post that until there is consensus about the general
availability of migrate disable, but for those who want to play with it
I've pushed the resulting combo out to:
git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git highmem
For testing I've tweaked a few places to use the new _local() variants
and it survived testing so far and I've verified that there is actual
preemption which means zap/restore of the thread local kmaps.
Thanks,
tglx
^ permalink raw reply
* RE: [PATCH 3/9] iov_iter: refactor rw_copy_check_uvector and import_iovec
From: David Laight @ 2020-09-23 14:38 UTC (permalink / raw)
To: 'Al Viro', Christoph Hellwig
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
Arnd Bergmann, linuxppc-dev@lists.ozlabs.org,
linux-block@vger.kernel.org, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton, Linus Torvalds
In-Reply-To: <20200923141654.GJ3421308@ZenIV.linux.org.uk>
From: Al Viro
> Sent: 23 September 2020 15:17
>
> On Wed, Sep 23, 2020 at 08:05:41AM +0200, Christoph Hellwig wrote:
>
> > +struct iovec *iovec_from_user(const struct iovec __user *uvec,
> > + unsigned long nr_segs, unsigned long fast_segs,
>
> Hmm... For fast_segs unsigned long had always been ridiculous
> (4G struct iovec on caller stack frame?), but that got me wondering about
> nr_segs and I wish I'd thought of that when introducing import_iovec().
>
> The thing is, import_iovec() takes unsigned int there. Which is fine
> (hell, the maximal value that can be accepted in 1024), except that
> we do pass unsigned long syscall argument to it in some places.
It will make diddly-squit difference.
The parameters end up in registers on most calling conventions.
Plausibly you get an extra 'REX' byte on x86 for the 64bit value.
What you want to avoid is explicit sign/zero extension and value
masking after arithmetic.
On x86-64 the 'horrid' type is actually 'signed int'.
It often needs sign extending to 64bits (eg when being
used as an array subscript).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH 3/9] iov_iter: refactor rw_copy_check_uvector and import_iovec
From: Al Viro @ 2020-09-23 14:40 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-aio, linux-mips, David Howells, linux-mm, keyrings,
sparclinux, linux-arch, linux-s390, linux-scsi, Linus Torvalds,
Arnd Bergmann, linux-block, io-uring, linux-arm-kernel,
Jens Axboe, linux-parisc, netdev, linux-kernel,
linux-security-module, David Laight, linux-fsdevel, Andrew Morton,
linuxppc-dev
In-Reply-To: <20200923141654.GJ3421308@ZenIV.linux.org.uk>
On Wed, Sep 23, 2020 at 03:16:54PM +0100, Al Viro wrote:
> On Wed, Sep 23, 2020 at 08:05:41AM +0200, Christoph Hellwig wrote:
>
> > +struct iovec *iovec_from_user(const struct iovec __user *uvec,
> > + unsigned long nr_segs, unsigned long fast_segs,
>
> Hmm... For fast_segs unsigned long had always been ridiculous
> (4G struct iovec on caller stack frame?), but that got me wondering about
> nr_segs and I wish I'd thought of that when introducing import_iovec().
>
> The thing is, import_iovec() takes unsigned int there. Which is fine
> (hell, the maximal value that can be accepted in 1024), except that
> we do pass unsigned long syscall argument to it in some places.
>
> E.g. vfs_readv() quietly truncates vlen to 32 bits, and vlen can
> come unchanged through sys_readv() -> do_readv() -> vfs_readv().
> With unsigned long passed by syscall glue.
>
> AFAICS, passing 4G+1 as the third argument to readv(2) on 64bit box
> will be quietly treated as 1 these days. Which would be fine, except
> that before "switch {compat_,}do_readv_writev() to {compat_,}import_iovec()"
> it used to fail with -EINVAL.
>
> Userland, BTW, describes readv(2) iovcnt as int; process_vm_readv(),
> OTOH, has these counts unsigned long from the userland POV...
>
> I suppose we ought to switch import_iovec() to unsigned long for nr_segs ;-/
> Strictly speaking that had been a userland ABI change, even though nothing
> except regression tests checking for expected errors would've been likely
> to notice. And it looks like no regression tests covered that one...
>
> Linus, does that qualify for your "if no userland has noticed the change,
> it's not a breakage"?
Egads... We have sys_readv() with unsigned long for file descriptor, since
1.3.31 when it had been introduced. And originally it did comparison with
NR_OPEN right in sys_readv(). Then in 2.1.60 it had been switched to
fget(), which used to take unsigned long at that point. And in 2.1.90pre1
it went unsigned int, so non-zero upper 32 bits in readv(2) first argument
ceased to cause EBADF...
Of course, libc had it as int fd all along.
^ permalink raw reply
* Re: [PATCH 3/9] iov_iter: refactor rw_copy_check_uvector and import_iovec
From: Al Viro @ 2020-09-23 14:49 UTC (permalink / raw)
To: David Laight
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, Linus Torvalds, Arnd Bergmann,
linux-block@vger.kernel.org, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <200cf2b9ce5e408f8838948fda7ce9a0@AcuMS.aculab.com>
On Wed, Sep 23, 2020 at 02:38:24PM +0000, David Laight wrote:
> From: Al Viro
> > Sent: 23 September 2020 15:17
> >
> > On Wed, Sep 23, 2020 at 08:05:41AM +0200, Christoph Hellwig wrote:
> >
> > > +struct iovec *iovec_from_user(const struct iovec __user *uvec,
> > > + unsigned long nr_segs, unsigned long fast_segs,
> >
> > Hmm... For fast_segs unsigned long had always been ridiculous
> > (4G struct iovec on caller stack frame?), but that got me wondering about
> > nr_segs and I wish I'd thought of that when introducing import_iovec().
> >
> > The thing is, import_iovec() takes unsigned int there. Which is fine
> > (hell, the maximal value that can be accepted in 1024), except that
> > we do pass unsigned long syscall argument to it in some places.
>
> It will make diddly-squit difference.
> The parameters end up in registers on most calling conventions.
> Plausibly you get an extra 'REX' byte on x86 for the 64bit value.
> What you want to avoid is explicit sign/zero extension and value
> masking after arithmetic.
Don't tell me what I want; your telepathic abilities are consistently sucky.
I am *NOT* talking about microoptimization here. I have described
the behaviour change of syscall caused by commit 5 years ago. Which is
generally considered a problem. Then I asked whether that behaviour
change would fall under the "if nobody noticed, it's not a userland ABI
breakage" exception.
Could you show me the point where I have expressed concerns about
the quality of amd64 code generated for that thing, before or after
the change in question?
^ permalink raw reply
* Re: [PATCH 5/9] fs: remove various compat readv/writev helpers
From: Al Viro @ 2020-09-23 14:59 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-aio, linux-mips, David Howells, linux-mm, keyrings,
sparclinux, linux-arch, linux-s390, linux-scsi, Arnd Bergmann,
linux-block, io-uring, linux-arm-kernel, Jens Axboe, linux-parisc,
netdev, linux-kernel, linux-security-module, David Laight,
linux-fsdevel, Andrew Morton, linuxppc-dev
In-Reply-To: <20200923143251.GA14062@lst.de>
On Wed, Sep 23, 2020 at 04:32:51PM +0200, Christoph Hellwig wrote:
> On Wed, Sep 23, 2020 at 03:25:49PM +0100, Al Viro wrote:
> > On Wed, Sep 23, 2020 at 08:05:43AM +0200, Christoph Hellwig wrote:
> > > COMPAT_SYSCALL_DEFINE3(readv, compat_ulong_t, fd,
> > > - const struct compat_iovec __user *,vec,
> > > + const struct iovec __user *, vec,
> >
> > Um... Will it even compile?
> >
> > > #ifdef __ARCH_WANT_COMPAT_SYS_PREADV64
> > > COMPAT_SYSCALL_DEFINE4(preadv64, unsigned long, fd,
> > > - const struct compat_iovec __user *,vec,
> > > + const struct iovec __user *, vec,
> >
> > Ditto. Look into include/linux/compat.h and you'll see
> >
> > asmlinkage long compat_sys_preadv64(unsigned long fd,
> > const struct compat_iovec __user *vec,
> > unsigned long vlen, loff_t pos);
> >
> > How does that manage to avoid the compiler screaming bloody
> > murder?
>
> That's a very good question. But it does not just compile but actually
> works. Probably because all the syscall wrappers mean that we don't
> actually generate the normal names. I just tried this:
>
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -468,7 +468,7 @@ asmlinkage long sys_lseek(unsigned int fd, off_t offset,
> asmlinkage long sys_read(unsigned int fd, char __user *buf, size_t count);
> asmlinkage long sys_write(unsigned int fd, const char __user *buf,
> size_t count);
> -asmlinkage long sys_readv(unsigned long fd,
> +asmlinkage long sys_readv(void *fd,
>
> for fun, and the compiler doesn't care either..
Try to build it for sparc or ppc...
^ permalink raw reply
* Re: [patch RFC 00/15] mm/highmem: Provide a preemptible variant of kmap_atomic & friends
From: Steven Rostedt @ 2020-09-23 15:52 UTC (permalink / raw)
To: peterz
Cc: Juri Lelli, David Airlie, Sebastian Andrzej Siewior,
Joonas Lahtinen, dri-devel, linux-mips, Ben Segall, Max Filippov,
Guo Ren, linux-sparc, Vincent Chen, Will Deacon, Ard Biesheuvel,
linux-arch, Vincent Guittot, Herbert Xu, the arch/x86 maintainers,
Russell King, linux-csky, Mel Gorman,
open list:SYNOPSYS ARC ARCHITECTURE, linux-xtensa, Paul McKenney,
intel-gfx, linuxppc-dev, Jani Nikula, Greentime Hu, Rodrigo Vivi,
Thomas Gleixner, Dietmar Eggemann, Linux ARM, Chris Zankel,
Michal Simek, Thomas Bogendoerfer, Nick Hu, Linux-MM,
Linus Torvalds, LKML, Arnd Bergmann, Daniel Vetter, Vineet Gupta,
Paul Mackerras, Andrew Morton, Daniel Bristot de Oliveira,
David S. Miller
In-Reply-To: <20200923084032.GU1362448@hirez.programming.kicks-ass.net>
On Wed, 23 Sep 2020 10:40:32 +0200
peterz@infradead.org wrote:
> However, with migrate_disable() we can have each task preempted in a
> migrate_disable() region, worse we can stack them all on the _same_ CPU
> (super ridiculous odds, sure). And then we end up only able to run one
> task, with the rest of the CPUs picking their nose.
What if we just made migrate_disable() a local_lock() available for !RT?
I mean make it a priority inheritance PER CPU lock.
That is, no two tasks could do a migrate_disable() on the same CPU? If
one task does a migrate_disable() and then gets preempted and the
preempting task does a migrate_disable() on the same CPU, it will block
and wait for the first task to do a migrate_enable().
No two tasks on the same CPU could enter the migrate_disable() section
simultaneously, just like no two tasks could enter a preempt_disable()
section.
In essence, we just allow local_lock() to be used for both RT and !RT.
Perhaps make migrate_disable() an anonymous local_lock()?
This should lower the SHC in theory, if you can't have stacked migrate
disables on the same CPU.
-- Steve
^ permalink raw reply
* Re: [PATCH v2] i2c: cpm: Fix i2c_ram structure
From: Wolfram Sang @ 2020-09-23 16:08 UTC (permalink / raw)
To: nicolas.vincent; +Cc: linuxppc-dev, linux-i2c
In-Reply-To: <20200923140840.8700-1-nicolas.vincent@vossloh.com>
[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]
On Wed, Sep 23, 2020 at 04:08:40PM +0200, nico.vince@gmail.com wrote:
> From: Nicolas VINCENT <nicolas.vincent@vossloh.com>
>
> the i2c_ram structure is missing the sdmatmp field mentionned in
> datasheet for MPC8272 at paragraph 36.5. With this field missing, the
> hardware would write past the allocated memory done through
> cpm_muram_alloc for the i2c_ram structure and land in memory allocated
> for the buffers descriptors corrupting the cbd_bufaddr field. Since this
> field is only set during setup(), the first i2c transaction would work
> and the following would send data read from an arbitrary memory
> location.
>
> Signed-off-by: Nicolas VINCENT <nicolas.vincent@vossloh.com>
Thanks!
Is someone able to identify a Fixes: tag I could add?
> ---
> drivers/i2c/busses/i2c-cpm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
> index 1213e1932ccb..24d584a1c9a7 100644
> --- a/drivers/i2c/busses/i2c-cpm.c
> +++ b/drivers/i2c/busses/i2c-cpm.c
> @@ -65,6 +65,9 @@ struct i2c_ram {
> char res1[4]; /* Reserved */
> ushort rpbase; /* Relocation pointer */
> char res2[2]; /* Reserved */
> + /* The following elements are only for CPM2 */
> + char res3[4]; /* Reserved */
> + uint sdmatmp; /* Internal */
> };
>
> #define I2COM_START 0x80
> --
> 2.17.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 5/9] fs: remove various compat readv/writev helpers
From: Al Viro @ 2020-09-23 16:38 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-aio, linux-mips, David Howells, linux-mm, keyrings,
sparclinux, linux-arch, linux-s390, linux-scsi, Arnd Bergmann,
linux-block, io-uring, linux-arm-kernel, Jens Axboe, linux-parisc,
netdev, linux-kernel, linux-security-module, David Laight,
linux-fsdevel, Andrew Morton, linuxppc-dev
In-Reply-To: <20200923145901.GN3421308@ZenIV.linux.org.uk>
On Wed, Sep 23, 2020 at 03:59:01PM +0100, Al Viro wrote:
> > That's a very good question. But it does not just compile but actually
> > works. Probably because all the syscall wrappers mean that we don't
> > actually generate the normal names. I just tried this:
> >
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -468,7 +468,7 @@ asmlinkage long sys_lseek(unsigned int fd, off_t offset,
> > asmlinkage long sys_read(unsigned int fd, char __user *buf, size_t count);
> > asmlinkage long sys_write(unsigned int fd, const char __user *buf,
> > size_t count);
> > -asmlinkage long sys_readv(unsigned long fd,
> > +asmlinkage long sys_readv(void *fd,
> >
> > for fun, and the compiler doesn't care either..
>
> Try to build it for sparc or ppc...
FWIW, declarations in syscalls.h used to serve 4 purposes:
1) syscall table initializers needed symbols declared
2) direct calls needed the same
3) catching mismatches between the declarations and definitions
4) centralized list of all syscalls
(2) has been (thankfully) reduced for some time; in any case, ksys_... is
used for the remaining ones.
(1) and (3) are served by syscalls.h in architectures other than x86, arm64
and s390. On those 3 (1) is done otherwise (near the syscall table initializer)
and (3) is not done at all.
I wonder if we should do something like
SYSCALL_DECLARE3(readv, unsigned long, fd, const struct iovec __user *, vec,
unsigned long, vlen);
in syscalls.h instead, and not under that ifdef.
Let it expand to declaration of sys_...() in generic case and, on x86, into
__do_sys_...() and __ia32_sys_...()/__x64_sys_...(), with types matching
what SYSCALL_DEFINE ends up using.
Similar macro would cover compat_sys_...() declarations. That would
restore mismatch checking for x86 and friends. AFAICS, the cost wouldn't
be terribly high - cpp would have more to chew through in syscalls.h,
but it shouldn't be all that costly. Famous last words, of course...
Does anybody see fundamental problems with that?
^ permalink raw reply
* Re: [PATCH 5/9] fs: remove various compat readv/writev helpers
From: Al Viro @ 2020-09-23 17:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-aio, linux-mips, David Howells, linux-mm, keyrings,
sparclinux, linux-arch, linux-s390, linux-scsi, Arnd Bergmann,
linux-block, io-uring, linux-arm-kernel, Jens Axboe, linux-parisc,
netdev, linux-kernel, linux-security-module, David Laight,
linux-fsdevel, Andrew Morton, linuxppc-dev
In-Reply-To: <20200923163831.GO3421308@ZenIV.linux.org.uk>
On Wed, Sep 23, 2020 at 05:38:31PM +0100, Al Viro wrote:
> On Wed, Sep 23, 2020 at 03:59:01PM +0100, Al Viro wrote:
>
> > > That's a very good question. But it does not just compile but actually
> > > works. Probably because all the syscall wrappers mean that we don't
> > > actually generate the normal names. I just tried this:
> > >
> > > --- a/include/linux/syscalls.h
> > > +++ b/include/linux/syscalls.h
> > > @@ -468,7 +468,7 @@ asmlinkage long sys_lseek(unsigned int fd, off_t offset,
> > > asmlinkage long sys_read(unsigned int fd, char __user *buf, size_t count);
> > > asmlinkage long sys_write(unsigned int fd, const char __user *buf,
> > > size_t count);
> > > -asmlinkage long sys_readv(unsigned long fd,
> > > +asmlinkage long sys_readv(void *fd,
> > >
> > > for fun, and the compiler doesn't care either..
> >
> > Try to build it for sparc or ppc...
>
> FWIW, declarations in syscalls.h used to serve 4 purposes:
> 1) syscall table initializers needed symbols declared
> 2) direct calls needed the same
> 3) catching mismatches between the declarations and definitions
> 4) centralized list of all syscalls
>
> (2) has been (thankfully) reduced for some time; in any case, ksys_... is
> used for the remaining ones.
>
> (1) and (3) are served by syscalls.h in architectures other than x86, arm64
> and s390. On those 3 (1) is done otherwise (near the syscall table initializer)
> and (3) is not done at all.
>
> I wonder if we should do something like
>
> SYSCALL_DECLARE3(readv, unsigned long, fd, const struct iovec __user *, vec,
> unsigned long, vlen);
> in syscalls.h instead, and not under that ifdef.
>
> Let it expand to declaration of sys_...() in generic case and, on x86, into
> __do_sys_...() and __ia32_sys_...()/__x64_sys_...(), with types matching
> what SYSCALL_DEFINE ends up using.
>
> Similar macro would cover compat_sys_...() declarations. That would
> restore mismatch checking for x86 and friends. AFAICS, the cost wouldn't
> be terribly high - cpp would have more to chew through in syscalls.h,
> but it shouldn't be all that costly. Famous last words, of course...
>
> Does anybody see fundamental problems with that?
Just to make it clear - I do not propose to fold that into this series;
there we just need to keep those declarations in sync with fs/read_write.c
^ permalink raw reply
* Re: [PATCH 5/9] fs: remove various compat readv/writev helpers
From: Brian Gerst @ 2020-09-23 17:08 UTC (permalink / raw)
To: Al Viro
Cc: linux-aio, open list:BROADCOM NVRAM DRIVER, David Howells,
Linux-MM, keyrings, sparclinux, Christoph Hellwig, linux-arch,
linux-s390, linux-scsi, Arnd Bergmann, linux-block, io-uring,
Linux ARM, Jens Axboe, Parisc List, netdev,
Linux Kernel Mailing List, linux-security-module, David Laight,
Linux FS-devel Mailing List, Andrew Morton, linuxppc-dev
In-Reply-To: <20200923163831.GO3421308@ZenIV.linux.org.uk>
On Wed, Sep 23, 2020 at 12:39 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Sep 23, 2020 at 03:59:01PM +0100, Al Viro wrote:
>
> > > That's a very good question. But it does not just compile but actually
> > > works. Probably because all the syscall wrappers mean that we don't
> > > actually generate the normal names. I just tried this:
> > >
> > > --- a/include/linux/syscalls.h
> > > +++ b/include/linux/syscalls.h
> > > @@ -468,7 +468,7 @@ asmlinkage long sys_lseek(unsigned int fd, off_t offset,
> > > asmlinkage long sys_read(unsigned int fd, char __user *buf, size_t count);
> > > asmlinkage long sys_write(unsigned int fd, const char __user *buf,
> > > size_t count);
> > > -asmlinkage long sys_readv(unsigned long fd,
> > > +asmlinkage long sys_readv(void *fd,
> > >
> > > for fun, and the compiler doesn't care either..
> >
> > Try to build it for sparc or ppc...
>
> FWIW, declarations in syscalls.h used to serve 4 purposes:
> 1) syscall table initializers needed symbols declared
> 2) direct calls needed the same
> 3) catching mismatches between the declarations and definitions
> 4) centralized list of all syscalls
>
> (2) has been (thankfully) reduced for some time; in any case, ksys_... is
> used for the remaining ones.
>
> (1) and (3) are served by syscalls.h in architectures other than x86, arm64
> and s390. On those 3 (1) is done otherwise (near the syscall table initializer)
> and (3) is not done at all.
>
> I wonder if we should do something like
>
> SYSCALL_DECLARE3(readv, unsigned long, fd, const struct iovec __user *, vec,
> unsigned long, vlen);
> in syscalls.h instead, and not under that ifdef.
>
> Let it expand to declaration of sys_...() in generic case and, on x86, into
> __do_sys_...() and __ia32_sys_...()/__x64_sys_...(), with types matching
> what SYSCALL_DEFINE ends up using.
>
> Similar macro would cover compat_sys_...() declarations. That would
> restore mismatch checking for x86 and friends. AFAICS, the cost wouldn't
> be terribly high - cpp would have more to chew through in syscalls.h,
> but it shouldn't be all that costly. Famous last words, of course...
>
> Does anybody see fundamental problems with that?
I think this would be a good idea. I have been working on a patchset
to clean up the conditional syscall handling (sys_ni.c), and conflicts
with the prototypes in syscalls.h have been getting in the way.
Having the prototypes use SYSCALL_DECLAREx(...) would solve that
issue.
--
Brian Gerst
^ permalink raw reply
* Re: [PATCH 5/9] fs: remove various compat readv/writev helpers
From: Christoph Hellwig @ 2020-09-23 17:46 UTC (permalink / raw)
To: Al Viro
Cc: linux-aio, linux-mips, David Howells, linux-mm, keyrings,
sparclinux, Christoph Hellwig, linux-arch, linux-s390, linux-scsi,
Arnd Bergmann, linux-block, io-uring, linux-arm-kernel,
Jens Axboe, linux-parisc, netdev, linux-kernel,
linux-security-module, David Laight, linux-fsdevel, Andrew Morton,
linuxppc-dev
In-Reply-To: <20200923170527.GQ3421308@ZenIV.linux.org.uk>
On Wed, Sep 23, 2020 at 06:05:27PM +0100, Al Viro wrote:
> On Wed, Sep 23, 2020 at 05:38:31PM +0100, Al Viro wrote:
> > On Wed, Sep 23, 2020 at 03:59:01PM +0100, Al Viro wrote:
> >
> > > > That's a very good question. But it does not just compile but actually
> > > > works. Probably because all the syscall wrappers mean that we don't
> > > > actually generate the normal names. I just tried this:
> > > >
> > > > --- a/include/linux/syscalls.h
> > > > +++ b/include/linux/syscalls.h
> > > > @@ -468,7 +468,7 @@ asmlinkage long sys_lseek(unsigned int fd, off_t offset,
> > > > asmlinkage long sys_read(unsigned int fd, char __user *buf, size_t count);
> > > > asmlinkage long sys_write(unsigned int fd, const char __user *buf,
> > > > size_t count);
> > > > -asmlinkage long sys_readv(unsigned long fd,
> > > > +asmlinkage long sys_readv(void *fd,
> > > >
> > > > for fun, and the compiler doesn't care either..
> > >
> > > Try to build it for sparc or ppc...
> >
> > FWIW, declarations in syscalls.h used to serve 4 purposes:
> > 1) syscall table initializers needed symbols declared
> > 2) direct calls needed the same
> > 3) catching mismatches between the declarations and definitions
> > 4) centralized list of all syscalls
> >
> > (2) has been (thankfully) reduced for some time; in any case, ksys_... is
> > used for the remaining ones.
> >
> > (1) and (3) are served by syscalls.h in architectures other than x86, arm64
> > and s390. On those 3 (1) is done otherwise (near the syscall table initializer)
> > and (3) is not done at all.
> >
> > I wonder if we should do something like
> >
> > SYSCALL_DECLARE3(readv, unsigned long, fd, const struct iovec __user *, vec,
> > unsigned long, vlen);
> > in syscalls.h instead, and not under that ifdef.
> >
> > Let it expand to declaration of sys_...() in generic case and, on x86, into
> > __do_sys_...() and __ia32_sys_...()/__x64_sys_...(), with types matching
> > what SYSCALL_DEFINE ends up using.
> >
> > Similar macro would cover compat_sys_...() declarations. That would
> > restore mismatch checking for x86 and friends. AFAICS, the cost wouldn't
> > be terribly high - cpp would have more to chew through in syscalls.h,
> > but it shouldn't be all that costly. Famous last words, of course...
> >
> > Does anybody see fundamental problems with that?
>
> Just to make it clear - I do not propose to fold that into this series;
> there we just need to keep those declarations in sync with fs/read_write.c
Agreed. The above idea generally sounds sane to me.
^ permalink raw reply
* Re: [PATCH v2] i2c: cpm: Fix i2c_ram structure
From: Christophe Leroy @ 2020-09-23 18:27 UTC (permalink / raw)
To: Wolfram Sang, nicolas.vincent; +Cc: linuxppc-dev, linux-i2c
In-Reply-To: <20200923160829.GB6697@ninjato>
Le 23/09/2020 à 18:08, Wolfram Sang a écrit :
> On Wed, Sep 23, 2020 at 04:08:40PM +0200, nico.vince@gmail.com wrote:
>> From: Nicolas VINCENT <nicolas.vincent@vossloh.com>
>>
>> the i2c_ram structure is missing the sdmatmp field mentionned in
>> datasheet for MPC8272 at paragraph 36.5. With this field missing, the
>> hardware would write past the allocated memory done through
>> cpm_muram_alloc for the i2c_ram structure and land in memory allocated
>> for the buffers descriptors corrupting the cbd_bufaddr field. Since this
>> field is only set during setup(), the first i2c transaction would work
>> and the following would send data read from an arbitrary memory
>> location.
>>
>> Signed-off-by: Nicolas VINCENT <nicolas.vincent@vossloh.com>
>
> Thanks!
>
> Is someone able to identify a Fixes: tag I could add?
I'd suggest
Fixes: 61045dbe9d8d ("i2c: Add support for I2C bus on Freescale
CPM1/CPM2 controllers")
Christophe
>
>> ---
>> drivers/i2c/busses/i2c-cpm.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
>> index 1213e1932ccb..24d584a1c9a7 100644
>> --- a/drivers/i2c/busses/i2c-cpm.c
>> +++ b/drivers/i2c/busses/i2c-cpm.c
>> @@ -65,6 +65,9 @@ struct i2c_ram {
>> char res1[4]; /* Reserved */
>> ushort rpbase; /* Relocation pointer */
>> char res2[2]; /* Reserved */
>> + /* The following elements are only for CPM2 */
>> + char res3[4]; /* Reserved */
>> + uint sdmatmp; /* Internal */
>> };
>>
>> #define I2COM_START 0x80
>> --
>> 2.17.1
>>
^ permalink raw reply
* Re: [PATCH 5/9] fs: remove various compat readv/writev helpers
From: Arnd Bergmann @ 2020-09-23 18:45 UTC (permalink / raw)
To: Al Viro
Cc: linux-aio, open list:BROADCOM NVRAM DRIVER, David Howells,
Linux-MM, keyrings, sparclinux, Christoph Hellwig, linux-arch,
linux-s390, linux-scsi, linux-block, io-uring, Linux ARM,
Jens Axboe, Parisc List, Networking, linux-kernel@vger.kernel.org,
LSM List, David Laight, Linux FS-devel Mailing List,
Andrew Morton, linuxppc-dev
In-Reply-To: <20200923163831.GO3421308@ZenIV.linux.org.uk>
On Wed, Sep 23, 2020 at 6:38 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> I wonder if we should do something like
>
> SYSCALL_DECLARE3(readv, unsigned long, fd, const struct iovec __user *, vec,
> unsigned long, vlen);
> in syscalls.h instead, and not under that ifdef.
>
> Let it expand to declaration of sys_...() in generic case and, on x86, into
> __do_sys_...() and __ia32_sys_...()/__x64_sys_...(), with types matching
> what SYSCALL_DEFINE ends up using.
>
> Similar macro would cover compat_sys_...() declarations. That would
> restore mismatch checking for x86 and friends. AFAICS, the cost wouldn't
> be terribly high - cpp would have more to chew through in syscalls.h,
> but it shouldn't be all that costly. Famous last words, of course...
>
> Does anybody see fundamental problems with that?
I've had some ideas along those lines in the past and I think it should work.
As a variation of this, the SYSCALL_DEFINEx() macros could go away
entirely, leaving only the macro instantiations from the header to
require that syntax. It would require first changing the remaining
architectures to build the syscall table from C code instead of
assembler though.
Regardless of that, another advantage of having the SYSCALL_DECLAREx()
would be the ability to include that header file from elsewhere with a different
macro definition to create a machine-readable version of the interface when
combined with the syscall.tbl files. This could be used to create a user
space stub for calling into the low-level syscall regardless of the
libc interfaces,
or for synchronizing the interfaces with strace, qemu-user, or anything that
needs to deal with the low-level interface.
Arnd
^ permalink raw reply
* Re: [PATCH 5/9] fs: remove various compat readv/writev helpers
From: Al Viro @ 2020-09-23 19:47 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-aio, open list:BROADCOM NVRAM DRIVER, David Howells,
Linux-MM, keyrings, sparclinux, Christoph Hellwig, linux-arch,
linux-s390, linux-scsi, linux-block, io-uring, Linux ARM,
Jens Axboe, Parisc List, Networking, linux-kernel@vger.kernel.org,
LSM List, David Laight, Linux FS-devel Mailing List,
Andrew Morton, linuxppc-dev
In-Reply-To: <CAK8P3a3nkLUOkR+jwz2_2LcYTUTqdVf8JOtZqKWbtEDotNhFZA@mail.gmail.com>
On Wed, Sep 23, 2020 at 08:45:51PM +0200, Arnd Bergmann wrote:
> On Wed, Sep 23, 2020 at 6:38 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > I wonder if we should do something like
> >
> > SYSCALL_DECLARE3(readv, unsigned long, fd, const struct iovec __user *, vec,
> > unsigned long, vlen);
> > in syscalls.h instead, and not under that ifdef.
> >
> > Let it expand to declaration of sys_...() in generic case and, on x86, into
> > __do_sys_...() and __ia32_sys_...()/__x64_sys_...(), with types matching
> > what SYSCALL_DEFINE ends up using.
> >
> > Similar macro would cover compat_sys_...() declarations. That would
> > restore mismatch checking for x86 and friends. AFAICS, the cost wouldn't
> > be terribly high - cpp would have more to chew through in syscalls.h,
> > but it shouldn't be all that costly. Famous last words, of course...
> >
> > Does anybody see fundamental problems with that?
>
> I've had some ideas along those lines in the past and I think it should work.
>
> As a variation of this, the SYSCALL_DEFINEx() macros could go away
> entirely, leaving only the macro instantiations from the header to
> require that syntax. It would require first changing the remaining
> architectures to build the syscall table from C code instead of
> assembler though.
>
> Regardless of that, another advantage of having the SYSCALL_DECLAREx()
> would be the ability to include that header file from elsewhere with a different
> macro definition to create a machine-readable version of the interface when
> combined with the syscall.tbl files. This could be used to create a user
> space stub for calling into the low-level syscall regardless of the
> libc interfaces,
> or for synchronizing the interfaces with strace, qemu-user, or anything that
> needs to deal with the low-level interface.
FWIW, after playing with that for a while... Do we really want the
compat_sys_...() declarations to live in linux/compat.h? Most of
the users of that file don't want those; why not move them to
linux/syscalls.h?
Reason: there's a lot more users of linux/compat.h than those of
linux/syscalls.h - it's pulled by everything in the networking stack,
for starters...
^ 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