Re: [4.10, panic, regression] iscsi: null pointer deref at iscsi_tcp_segment_done+0x20d/0x2e0

[Chris Leech]

On Thu, Dec 22, 2016 at 05:50:12PM +1100, Dave Chinner wrote:
> On Wed, Dec 21, 2016 at 09:46:37PM -0800, Linus Torvalds wrote:
> > On Wed, Dec 21, 2016 at 9:13 PM, Dave Chinner  wrote:
> > >
> > > There may be deeper issues. I just started running scalability tests
> > > (e.g. 16-way fsmark create tests) and about a minute in I got a
> > > directory corruption reported - something I hadn't seen in the dev
> > > cycle at all.
> > 
> > By "in the dev cycle", do you mean your XFS changes, or have you been
> > tracking the merge cycle at least for some testing?
> 
> I mean the three months leading up to the 4.10 merge, when all the
> XFS changes were being tested against 4.9-rc kernels.
> 
> The iscsi problem showed up when I updated the base kernel from
> 4.9 to 4.10-current last week to test the pullreq I was going to
> send you. I've been bust with other stuff until now, so I didn't
> upgrade my working trees again until today in the hope the iscsi
> problem had already been found and fixed.
> 
> > > I unmounted the fs, mkfs'd it again, ran the
> > > workload again and about a minute in this fired:
> > >
> > > [628867.607417] [ cut here ]
> > > [628867.608603] WARNING: CPU: 2 PID: 16925 at mm/workingset.c:461 
> > > shadow_lru_isolate+0x171/0x220
> > 
> > Well, part of the changes during the merge window were the shadow
> > entry tracking changes that came in through Andrew's tree. Adding
> > Johannes Weiner to the participants.
> > 
> > > Now, this workload does not touch the page cache at all - it's
> > > entirely an XFS metadata workload, so it should not really be
> > > affecting the working set code.
> > 
> > Well, I suspect that anything that creates memory pressure will end up
> > triggering the working set code, so ..
> > 
> > That said, obviously memory corruption could be involved and result in
> > random issues too, but I wouldn't really expect that in this code.
> > 
> > It would probably be really useful to get more data points - is the
> > problem reliably in this area, or is it going to be random and all
> > over the place.
> 
> The iscsi problem is 100% reproducable. create a pair of iscsi luns,
> mkfs, run xfstests on them. iscsi fails a second after xfstests mounts
> the filesystems.
> 
> The test machine I'm having all these other problems on? stable and
> steady as a rock using PMEM devices. Moment I go to use /dev/vdc
> (i.e. run load/perf benchmarks) it starts falling over left, right
> and center.

I'm not reproducing any problems with xfstests running over iscsi_tcp
right now.  Two 10G luns exported from an LIO target, attached directly
to a test VM as sda/sdb and xfstests configured to use sda1/sdb1 as
TEST_DEV and SCRATCH_DEV.

The virtio scatterlist issue that popped right away for me is triggered
by an hdparm ioctl, which is being run by tuned on Fedora.  And that
actually seems to happen back on 4.9 as well :(

Chris

Re: [4.10, panic, regression] iscsi: null pointer deref at iscsi_tcp_segment_done+0x20d/0x2e0

[Chris Leech]

On Thu, Dec 22, 2016 at 05:50:12PM +1100, Dave Chinner wrote:
> On Wed, Dec 21, 2016 at 09:46:37PM -0800, Linus Torvalds wrote:
> > On Wed, Dec 21, 2016 at 9:13 PM, Dave Chinner  wrote:
> > >
> > > There may be deeper issues. I just started running scalability tests
> > > (e.g. 16-way fsmark create tests) and about a minute in I got a
> > > directory corruption reported - something I hadn't seen in the dev
> > > cycle at all.
> > 
> > By "in the dev cycle", do you mean your XFS changes, or have you been
> > tracking the merge cycle at least for some testing?
> 
> I mean the three months leading up to the 4.10 merge, when all the
> XFS changes were being tested against 4.9-rc kernels.
> 
> The iscsi problem showed up when I updated the base kernel from
> 4.9 to 4.10-current last week to test the pullreq I was going to
> send you. I've been bust with other stuff until now, so I didn't
> upgrade my working trees again until today in the hope the iscsi
> problem had already been found and fixed.
> 
> > > I unmounted the fs, mkfs'd it again, ran the
> > > workload again and about a minute in this fired:
> > >
> > > [628867.607417] [ cut here ]
> > > [628867.608603] WARNING: CPU: 2 PID: 16925 at mm/workingset.c:461 
> > > shadow_lru_isolate+0x171/0x220
> > 
> > Well, part of the changes during the merge window were the shadow
> > entry tracking changes that came in through Andrew's tree. Adding
> > Johannes Weiner to the participants.
> > 
> > > Now, this workload does not touch the page cache at all - it's
> > > entirely an XFS metadata workload, so it should not really be
> > > affecting the working set code.
> > 
> > Well, I suspect that anything that creates memory pressure will end up
> > triggering the working set code, so ..
> > 
> > That said, obviously memory corruption could be involved and result in
> > random issues too, but I wouldn't really expect that in this code.
> > 
> > It would probably be really useful to get more data points - is the
> > problem reliably in this area, or is it going to be random and all
> > over the place.
> 
> The iscsi problem is 100% reproducable. create a pair of iscsi luns,
> mkfs, run xfstests on them. iscsi fails a second after xfstests mounts
> the filesystems.
> 
> The test machine I'm having all these other problems on? stable and
> steady as a rock using PMEM devices. Moment I go to use /dev/vdc
> (i.e. run load/perf benchmarks) it starts falling over left, right
> and center.

I'm not reproducing any problems with xfstests running over iscsi_tcp
right now.  Two 10G luns exported from an LIO target, attached directly
to a test VM as sda/sdb and xfstests configured to use sda1/sdb1 as
TEST_DEV and SCRATCH_DEV.

The virtio scatterlist issue that popped right away for me is triggered
by an hdparm ioctl, which is being run by tuned on Fedora.  And that
actually seems to happen back on 4.9 as well :(

Chris

Re: [PATCH 11/12] crypto: atmel-authenc: add support to authenc(hmac(shaX),Y(aes)) modes

[kbuild test robot]

Hi Cyrille,

[auto build test ERROR on cryptodev/master]
[also build test ERROR on next-20161222]
[cannot apply to v4.9]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Cyrille-Pitchen/crypto-atmel-authenc-add-support-to-authenc-hmac-shaX-Y-aes-modes/20161223-012130
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All error/warnings (new ones prefixed by >>):

warning: (CRYPTO_DEV_ATMEL_AUTHENC) selects CRYPTO_DEV_ATMEL_SHA which has 
unmet direct dependencies (CRYPTO && CRYPTO_HW && ARCH_AT91)
>> drivers/crypto/atmel-aes.c:44:27: fatal error: atmel-authenc.h: No such file 
>> or directory
#include "atmel-authenc.h"
  ^
   compilation terminated.
--
>> drivers/crypto/atmel-sha.c:44:27: fatal error: atmel-authenc.h: No such file 
>> or directory
#include "atmel-authenc.h"
  ^
   compilation terminated.

vim +44 drivers/crypto/atmel-aes.c

38  #include 
39  #include 
40  #include 
41  #include 
42  #include 
43  #include "atmel-aes-regs.h"
  > 44  #include "atmel-authenc.h"
45  
46  #define ATMEL_AES_PRIORITY  300
47  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH 11/12] crypto: atmel-authenc: add support to authenc(hmac(shaX),Y(aes)) modes

[kbuild test robot]

Hi Cyrille,

[auto build test ERROR on cryptodev/master]
[also build test ERROR on next-20161222]
[cannot apply to v4.9]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Cyrille-Pitchen/crypto-atmel-authenc-add-support-to-authenc-hmac-shaX-Y-aes-modes/20161223-012130
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All error/warnings (new ones prefixed by >>):

warning: (CRYPTO_DEV_ATMEL_AUTHENC) selects CRYPTO_DEV_ATMEL_SHA which has 
unmet direct dependencies (CRYPTO && CRYPTO_HW && ARCH_AT91)
>> drivers/crypto/atmel-aes.c:44:27: fatal error: atmel-authenc.h: No such file 
>> or directory
#include "atmel-authenc.h"
  ^
   compilation terminated.
--
>> drivers/crypto/atmel-sha.c:44:27: fatal error: atmel-authenc.h: No such file 
>> or directory
#include "atmel-authenc.h"
  ^
   compilation terminated.

vim +44 drivers/crypto/atmel-aes.c

38  #include 
39  #include 
40  #include 
41  #include 
42  #include 
43  #include "atmel-aes-regs.h"
  > 44  #include "atmel-authenc.h"
45  
46  #define ATMEL_AES_PRIORITY  300
47  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH 1/3] NFC: trf7970a: add device tree option for 27MHz clock

[Rob Herring]

On Mon, Dec 19, 2016 at 5:23 PM, Geoff Lansberry  wrote:
> I can make that change, however, I worry that it may be a bit
> misleading, since there are only two supported clock frequencies, but
> a number like that to me implies that it could be set to any number
> you want.   I'm new at this, and so I'll go ahead and change it as you
> request, but I'd like to hear your thoughts on my concern.

Then the binding doc just needs to state what are the 2 valid frequencies.

Rob

Re: [PATCH 1/3] NFC: trf7970a: add device tree option for 27MHz clock

[Rob Herring]

On Mon, Dec 19, 2016 at 5:23 PM, Geoff Lansberry  wrote:
> I can make that change, however, I worry that it may be a bit
> misleading, since there are only two supported clock frequencies, but
> a number like that to me implies that it could be set to any number
> you want.   I'm new at this, and so I'll go ahead and change it as you
> request, but I'd like to hear your thoughts on my concern.

Then the binding doc just needs to state what are the 2 valid frequencies.

Rob

Re: [PATCH] pci: add kernel config option for disabling common PCI quirks

[Bjorn Helgaas]

On Thu, Dec 22, 2016 at 06:54:51AM +0100, John Crispin wrote:
> On 21/12/2016 15:26, Christoph Hellwig wrote:
> > On Wed, Dec 21, 2016 at 02:11:25PM +0100, John Crispin wrote:
> >> I can turn it into an enable patch that is selected by default.
> >>
> >> The current patch disables all those quirks that are used for x86/PC
> >> style machines and hence are not required in the embedded world.
> > 
> > Maybe we'll just need to reorganize the quirks so that most of them
> > arch in arch code or the affected drivers?
> 
> to be honest i have no opinion on this. I am currently trying to reduce
> the amount of patches that we have inside the LEDE tree. the patches
> were written by other people and then dumped on us. obviously i am
> interested to get this upstream with the least amount of effort. I am
> quite aware though that some patches will need an overhaul to be
> applicable for upstream. its not really my call if it is enough to make
> this an enable patch and review the quirks enabled by it or if the code
> needs to be moved.

We already have CONFIG_PCI_QUIRKS, which enables everything in
drivers/pci/quirks.c.  That file should contain quirks for devices
that may appear on any architecture, e.g., things on plug-in cards.

Quirks that are only applicable to one arch should be in the arch
directory, e.g., in arch/x86/pci/fixup.c.

If drivers/pci/quirks.c contains arch-specific quirks, we should move
those to the appropriate arch directory.

It looks like arch/x86/pci/fixup.c is currently compiled
unconditionally (on x86 with PCI), but we should probably make it
dependent on CONFIG_PCI_QUIRKS, since I think the infrastructure that
*calls* those quirks is only present when CONFIG_PCI_QUIRKS=y.

Bjorn

Re: [PATCH] pci: add kernel config option for disabling common PCI quirks

[Bjorn Helgaas]

On Thu, Dec 22, 2016 at 06:54:51AM +0100, John Crispin wrote:
> On 21/12/2016 15:26, Christoph Hellwig wrote:
> > On Wed, Dec 21, 2016 at 02:11:25PM +0100, John Crispin wrote:
> >> I can turn it into an enable patch that is selected by default.
> >>
> >> The current patch disables all those quirks that are used for x86/PC
> >> style machines and hence are not required in the embedded world.
> > 
> > Maybe we'll just need to reorganize the quirks so that most of them
> > arch in arch code or the affected drivers?
> 
> to be honest i have no opinion on this. I am currently trying to reduce
> the amount of patches that we have inside the LEDE tree. the patches
> were written by other people and then dumped on us. obviously i am
> interested to get this upstream with the least amount of effort. I am
> quite aware though that some patches will need an overhaul to be
> applicable for upstream. its not really my call if it is enough to make
> this an enable patch and review the quirks enabled by it or if the code
> needs to be moved.

We already have CONFIG_PCI_QUIRKS, which enables everything in
drivers/pci/quirks.c.  That file should contain quirks for devices
that may appear on any architecture, e.g., things on plug-in cards.

Quirks that are only applicable to one arch should be in the arch
directory, e.g., in arch/x86/pci/fixup.c.

If drivers/pci/quirks.c contains arch-specific quirks, we should move
those to the appropriate arch directory.

It looks like arch/x86/pci/fixup.c is currently compiled
unconditionally (on x86 with PCI), but we should probably make it
dependent on CONFIG_PCI_QUIRKS, since I think the infrastructure that
*calls* those quirks is only present when CONFIG_PCI_QUIRKS=y.

Bjorn

Re: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-type-adjustment" for INCR burst type

[Rob Herring]

On Mon, Dec 19, 2016 at 05:25:53PM +0800, Changming Huang wrote:
> New property "snps,incr-burst-type-adjustment = , " for USB3.0 DWC3.
> Field "x": 1/0 - undefined length INCR burst type enable or not;
> Field "y": INCR4/INCR8/INCR16/INCR32/INCR64/INCR128/INCR256 burst type.
> 
> While enabling undefined length INCR burst type and INCR16 burst type,
> get better write performance on NXP Layerscape platform:
> around 3% improvement (from 364MB/s to 375MB/s).
> 
> Signed-off-by: Changming Huang 
> ---
> Changes in v3:
>   - add new property for INCR burst in usb node.
> 
>  Documentation/devicetree/bindings/usb/dwc3.txt |5 +
>  arch/arm/boot/dts/ls1021a.dtsi |1 +
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi |3 +++
>  arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi |2 ++
>  4 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
> b/Documentation/devicetree/bindings/usb/dwc3.txt
> index e3e6983..8c405a3 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -55,6 +55,10 @@ Optional properties:
>   fladj_30mhz_sdbnd signal is invalid or incorrect.
>  
>   -  tx-fifo-resize: determines if the FIFO *has* to be 
> reallocated.
> + - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0
> + register, undefined length INCR burst type enable and INCRx type.
> + First field is for undefined length INCR burst type enable or not.
> + Second field is for largest INCRx type enabled.

Why do you need the first field? Is the 2nd field used if the 1st is 0? 
If not, then just use the presence of the property to enable or not.

Rob

Re: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-type-adjustment" for INCR burst type

[Rob Herring]

On Mon, Dec 19, 2016 at 05:25:53PM +0800, Changming Huang wrote:
> New property "snps,incr-burst-type-adjustment = , " for USB3.0 DWC3.
> Field "x": 1/0 - undefined length INCR burst type enable or not;
> Field "y": INCR4/INCR8/INCR16/INCR32/INCR64/INCR128/INCR256 burst type.
> 
> While enabling undefined length INCR burst type and INCR16 burst type,
> get better write performance on NXP Layerscape platform:
> around 3% improvement (from 364MB/s to 375MB/s).
> 
> Signed-off-by: Changming Huang 
> ---
> Changes in v3:
>   - add new property for INCR burst in usb node.
> 
>  Documentation/devicetree/bindings/usb/dwc3.txt |5 +
>  arch/arm/boot/dts/ls1021a.dtsi |1 +
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi |3 +++
>  arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi |2 ++
>  4 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
> b/Documentation/devicetree/bindings/usb/dwc3.txt
> index e3e6983..8c405a3 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -55,6 +55,10 @@ Optional properties:
>   fladj_30mhz_sdbnd signal is invalid or incorrect.
>  
>   -  tx-fifo-resize: determines if the FIFO *has* to be 
> reallocated.
> + - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0
> + register, undefined length INCR burst type enable and INCRx type.
> + First field is for undefined length INCR burst type enable or not.
> + Second field is for largest INCRx type enabled.

Why do you need the first field? Is the 2nd field used if the 1st is 0? 
If not, then just use the presence of the property to enable or not.

Rob

Re: [PATCH v2] fs: exec: apply CLOEXEC before changing dumpable task flags

[Eric W. Biederman]

Aleksa Sarai  writes:

> If you have a process that has set itself to be non-dumpable, and it
> then undergoes exec(2), any CLOEXEC file descriptors it has open are
> "exposed" during a race window between the dumpable flags of the process
> being reset for exec(2) and CLOEXEC being applied to the file
> descriptors. This can be exploited by a process by attempting to access
> /proc//fd/... during this window, without requiring CAP_SYS_PTRACE.
>
> The race in question is after set_dumpable has been (for get_link,
> though the trace is basically the same for readlink):
>
> [vfs]
> -> proc_pid_link_inode_operations.get_link
>-> proc_pid_get_link
>   -> proc_fd_access_allowed
>  -> ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
>
> Which will return 0, during the race window and CLOEXEC file descriptors
> will still be open during this window because do_close_on_exec has not
> been called yet. As a result, the ordering of these calls should be
> reversed to avoid this race window.
>
> This is of particular concern to container runtimes, where joining a
> PID namespace with file descriptors referring to the host filesystem
> can result in security issues (since PRCTL_SET_DUMPABLE doesn't protect
> against access of CLOEXEC file descriptors -- file descriptors which may
> reference filesystem objects the container shouldn't have access to).

That seems reasonable.  I was thinking cred_guard_mutex should handle
this case, but it obviously won't because only ptrace_attach takes that.
Sigh with enough cleanups the code might even become comprehensible and
correct in there.

I have dropped this onto my for-testing branch for now (so I don't
forget it) and after the chaos of the merge window ends I will forward
this along.

Eric


> Cc: d...@opencontainers.org
> Cc:  # v3.2+
> Reported-by: Michael Crosby 
> Signed-off-by: Aleksa Sarai 
> ---
>  fs/exec.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 4e497b9ee71e..b0a98ef03253 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -19,7 +19,7 @@
>   * current->executable is only used by the procfs.  This allows a dispatch
>   * table to check for several different types  of binary formats.  We keep
>   * trying until we recognize the file or we run out of supported binary
> - * formats. 
> + * formats.
>   */
>  
>  #include 
> @@ -1266,6 +1266,13 @@ int flush_old_exec(struct linux_binprm * bprm)
>   flush_thread();
>   current->personality &= ~bprm->per_clear;
>  
> + /*
> +  * We have to apply CLOEXEC before we change whether the process is
> +  * dumpable (in setup_new_exec) to avoid a race with a process in 
> userspace
> +  * trying to access the should-be-closed file descriptors of a process
> +  * undergoing exec(2).
> +  */
> + do_close_on_exec(current->files);
>   return 0;
>  
>  out:
> @@ -1315,7 +1322,6 @@ void setup_new_exec(struct linux_binprm * bprm)
>  group */
>   current->self_exec_id++;
>   flush_signal_handlers(current, 0);
> - do_close_on_exec(current->files);
>  }
>  EXPORT_SYMBOL(setup_new_exec);

Re: [PATCH v2] fs: exec: apply CLOEXEC before changing dumpable task flags

[Eric W. Biederman]

Aleksa Sarai  writes:

> If you have a process that has set itself to be non-dumpable, and it
> then undergoes exec(2), any CLOEXEC file descriptors it has open are
> "exposed" during a race window between the dumpable flags of the process
> being reset for exec(2) and CLOEXEC being applied to the file
> descriptors. This can be exploited by a process by attempting to access
> /proc//fd/... during this window, without requiring CAP_SYS_PTRACE.
>
> The race in question is after set_dumpable has been (for get_link,
> though the trace is basically the same for readlink):
>
> [vfs]
> -> proc_pid_link_inode_operations.get_link
>-> proc_pid_get_link
>   -> proc_fd_access_allowed
>  -> ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
>
> Which will return 0, during the race window and CLOEXEC file descriptors
> will still be open during this window because do_close_on_exec has not
> been called yet. As a result, the ordering of these calls should be
> reversed to avoid this race window.
>
> This is of particular concern to container runtimes, where joining a
> PID namespace with file descriptors referring to the host filesystem
> can result in security issues (since PRCTL_SET_DUMPABLE doesn't protect
> against access of CLOEXEC file descriptors -- file descriptors which may
> reference filesystem objects the container shouldn't have access to).

That seems reasonable.  I was thinking cred_guard_mutex should handle
this case, but it obviously won't because only ptrace_attach takes that.
Sigh with enough cleanups the code might even become comprehensible and
correct in there.

I have dropped this onto my for-testing branch for now (so I don't
forget it) and after the chaos of the merge window ends I will forward
this along.

Eric


> Cc: d...@opencontainers.org
> Cc:  # v3.2+
> Reported-by: Michael Crosby 
> Signed-off-by: Aleksa Sarai 
> ---
>  fs/exec.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 4e497b9ee71e..b0a98ef03253 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -19,7 +19,7 @@
>   * current->executable is only used by the procfs.  This allows a dispatch
>   * table to check for several different types  of binary formats.  We keep
>   * trying until we recognize the file or we run out of supported binary
> - * formats. 
> + * formats.
>   */
>  
>  #include 
> @@ -1266,6 +1266,13 @@ int flush_old_exec(struct linux_binprm * bprm)
>   flush_thread();
>   current->personality &= ~bprm->per_clear;
>  
> + /*
> +  * We have to apply CLOEXEC before we change whether the process is
> +  * dumpable (in setup_new_exec) to avoid a race with a process in 
> userspace
> +  * trying to access the should-be-closed file descriptors of a process
> +  * undergoing exec(2).
> +  */
> + do_close_on_exec(current->files);
>   return 0;
>  
>  out:
> @@ -1315,7 +1322,6 @@ void setup_new_exec(struct linux_binprm * bprm)
>  group */
>   current->self_exec_id++;
>   flush_signal_handlers(current, 0);
> - do_close_on_exec(current->files);
>  }
>  EXPORT_SYMBOL(setup_new_exec);

Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

[Andy Shevchenko]

On Thu, Dec 22, 2016 at 3:07 AM, Pierre-Louis Bossart
 wrote:
> On 12/21/16 5:05 PM, Stephen Boyd wrote:

>>> The name pmc_plt_clk_ follows the data sheet specification, where
>>> this convention is suggested:
>>>   PLT_CLK[2:0] - Camera
>>>   PLT_CLK[3] - Audio Codec
>>>   PLT_CLK[4] -
>>>   PLT_CLK[5] - COMMs

By the way, would I suggest to use same prefix as provider, i.e.
pmc_atom_plt_clk_%d ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V2 1/2] PM / Domains: Introduce domain-performance-states binding

[Rob Herring]

On Mon, Dec 12, 2016 at 04:26:18PM +0530, Viresh Kumar wrote:
> Some platforms have the capability to configure the performance state of
> their Power Domains. The performance levels are represented by positive
> integer values, a lower value represents lower performance state.
> 
> The power-domains until now were only concentrating on the idle state
> management of the device and this needs to change in order to reuse the
> infrastructure of power domains for active state management.
> 
> This patch adds binding to describe the performance states of a power
> domain.
> 
> If the consumers don't need the capability of switching to different
> domain performance states at runtime, then they can simply define their
> required domain performance state in their node directly. Otherwise the
> consumers can define their requirements with help of other
> infrastructure, for example the OPP table.
> 
> Signed-off-by: Viresh Kumar 
> ---
>  .../devicetree/bindings/power/power_domain.txt | 69 
> ++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt 
> b/Documentation/devicetree/bindings/power/power_domain.txt
> index 723e1ad937da..a456e0dc04e0 100644
> --- a/Documentation/devicetree/bindings/power/power_domain.txt
> +++ b/Documentation/devicetree/bindings/power/power_domain.txt
> @@ -38,6 +38,40 @@ phandle arguments (so called PM domain specifiers) of 
> length specified by the
>domain's idle states. In the absence of this property, the domain would be
>considered as capable of being powered-on or powered-off.
>  
> +- domain-performance-states : A phandle of the performance states node, which
> + defines all the performance states associated with a power
> + domain.
> +  The domain-performance-states property reflects the performance states of 
> this
> +  PM domain and not the performance states of the devices or sub-domains in 
> the
> +  PM domain. Devices and sub-domains have their own performance states, which
> +  are dependent on the performance state of the PM domain.
> +
> +* PM domain performance states node
> +
> +This describes the performance states of a PM domain.
> +
> +Required properties:
> +- compatible: Allow performance states to express their compatibility. It 
> should
> +  be: "domain-performance-state".
> +
> +- Performance state nodes: This node shall have one or more "Performance 
> State"
> +  nodes.
> +
> +* Performance state node
> +
> +Required properties:
> +- performance-level: A positive integer value representing the performance 
> level
> +  associated with a performance state. The integer value '1' represents the
> +  lowest performance level and the highest value represents the highest
> +  performance level.
> +
> +Optional properties:
> +- domain-microvolt: voltage in micro Volts.
> +
> +  A single regulator's voltage is specified with an array of size one or 
> three.
> +  Single entry is for target voltage and three entries are for  max>
> +  voltages.
> +
>  Example:
>  
>   power: power-controller@1234 {
> @@ -118,4 +152,39 @@ The node above defines a typical PM domain consumer 
> device, which is located
>  inside a PM domain with index 0 of a power controller represented by a node
>  with the label "power".
>  
> +Optional properties:
> +- domain-performance-state: A phandle of a Performance state node.
> +
> +Example:
> +
> + parent: power-controller@1234 {
> + compatible = "foo,power-controller";
> + reg = <0x1234 0x1000>;
> + #power-domain-cells = <0>;
> + domain-performance-states = <_perf_states>;
> + };
> +
> + domain_perf_states: performance_states {

If you want to have performance states for a domain in DT, then you need 
to actually have a node for the domain in DT. Then this should be a 
child of the domain. I wouldn't think non-CPU domain performance states 
will be common across domains.

> + compatible = "domain-performance-state";
> + domain_perf_state1: pstate@1 {

A unit address should have a reg property.

> + performance-level = <1>;
> + domain-microvolt = <97 975000 985000>;
> + };
> + domain_perf_state2: pstate@2 {
> + performance-level = <2>;
> + domain-microvolt = <100 1075000 1085000>;
> + };
> + domain_perf_state3: pstate@3 {
> + performance-level = <3>;
> + domain-microvolt = <110 1175000 1185000>;
> + };
> + }
> +
> + leaky-device@1235 {
> + compatible = "foo,i-leak-current";
> + reg = <0x1235 0x1000>;
> + power-domains = < 0>;
> + domain-performance-state = <_perf_state2>;

domain-performance-state and domain-performance-states are too similar 
in name. 

Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

[Andy Shevchenko]

On Thu, Dec 22, 2016 at 3:07 AM, Pierre-Louis Bossart
 wrote:
> On 12/21/16 5:05 PM, Stephen Boyd wrote:

>>> The name pmc_plt_clk_ follows the data sheet specification, where
>>> this convention is suggested:
>>>   PLT_CLK[2:0] - Camera
>>>   PLT_CLK[3] - Audio Codec
>>>   PLT_CLK[4] -
>>>   PLT_CLK[5] - COMMs

By the way, would I suggest to use same prefix as provider, i.e.
pmc_atom_plt_clk_%d ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V2 1/2] PM / Domains: Introduce domain-performance-states binding

[Rob Herring]

On Mon, Dec 12, 2016 at 04:26:18PM +0530, Viresh Kumar wrote:
> Some platforms have the capability to configure the performance state of
> their Power Domains. The performance levels are represented by positive
> integer values, a lower value represents lower performance state.
> 
> The power-domains until now were only concentrating on the idle state
> management of the device and this needs to change in order to reuse the
> infrastructure of power domains for active state management.
> 
> This patch adds binding to describe the performance states of a power
> domain.
> 
> If the consumers don't need the capability of switching to different
> domain performance states at runtime, then they can simply define their
> required domain performance state in their node directly. Otherwise the
> consumers can define their requirements with help of other
> infrastructure, for example the OPP table.
> 
> Signed-off-by: Viresh Kumar 
> ---
>  .../devicetree/bindings/power/power_domain.txt | 69 
> ++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt 
> b/Documentation/devicetree/bindings/power/power_domain.txt
> index 723e1ad937da..a456e0dc04e0 100644
> --- a/Documentation/devicetree/bindings/power/power_domain.txt
> +++ b/Documentation/devicetree/bindings/power/power_domain.txt
> @@ -38,6 +38,40 @@ phandle arguments (so called PM domain specifiers) of 
> length specified by the
>domain's idle states. In the absence of this property, the domain would be
>considered as capable of being powered-on or powered-off.
>  
> +- domain-performance-states : A phandle of the performance states node, which
> + defines all the performance states associated with a power
> + domain.
> +  The domain-performance-states property reflects the performance states of 
> this
> +  PM domain and not the performance states of the devices or sub-domains in 
> the
> +  PM domain. Devices and sub-domains have their own performance states, which
> +  are dependent on the performance state of the PM domain.
> +
> +* PM domain performance states node
> +
> +This describes the performance states of a PM domain.
> +
> +Required properties:
> +- compatible: Allow performance states to express their compatibility. It 
> should
> +  be: "domain-performance-state".
> +
> +- Performance state nodes: This node shall have one or more "Performance 
> State"
> +  nodes.
> +
> +* Performance state node
> +
> +Required properties:
> +- performance-level: A positive integer value representing the performance 
> level
> +  associated with a performance state. The integer value '1' represents the
> +  lowest performance level and the highest value represents the highest
> +  performance level.
> +
> +Optional properties:
> +- domain-microvolt: voltage in micro Volts.
> +
> +  A single regulator's voltage is specified with an array of size one or 
> three.
> +  Single entry is for target voltage and three entries are for  max>
> +  voltages.
> +
>  Example:
>  
>   power: power-controller@1234 {
> @@ -118,4 +152,39 @@ The node above defines a typical PM domain consumer 
> device, which is located
>  inside a PM domain with index 0 of a power controller represented by a node
>  with the label "power".
>  
> +Optional properties:
> +- domain-performance-state: A phandle of a Performance state node.
> +
> +Example:
> +
> + parent: power-controller@1234 {
> + compatible = "foo,power-controller";
> + reg = <0x1234 0x1000>;
> + #power-domain-cells = <0>;
> + domain-performance-states = <_perf_states>;
> + };
> +
> + domain_perf_states: performance_states {

If you want to have performance states for a domain in DT, then you need 
to actually have a node for the domain in DT. Then this should be a 
child of the domain. I wouldn't think non-CPU domain performance states 
will be common across domains.

> + compatible = "domain-performance-state";
> + domain_perf_state1: pstate@1 {

A unit address should have a reg property.

> + performance-level = <1>;
> + domain-microvolt = <97 975000 985000>;
> + };
> + domain_perf_state2: pstate@2 {
> + performance-level = <2>;
> + domain-microvolt = <100 1075000 1085000>;
> + };
> + domain_perf_state3: pstate@3 {
> + performance-level = <3>;
> + domain-microvolt = <110 1175000 1185000>;
> + };
> + }
> +
> + leaky-device@1235 {
> + compatible = "foo,i-leak-current";
> + reg = <0x1235 0x1000>;
> + power-domains = < 0>;
> + domain-performance-state = <_perf_state2>;

domain-performance-state and domain-performance-states are too similar 
in name. The property here should 

Re: [PATCH v3 2/2] crypto: mediatek - add DT bindings documentation

[Rob Herring]

On Mon, Dec 19, 2016 at 10:20:45AM +0800, Ryder Lee wrote:
> Add DT bindings documentation for the crypto driver
> 
> Signed-off-by: Ryder Lee 
> ---
>  .../devicetree/bindings/crypto/mediatek-crypto.txt | 27 
> ++
>  1 file changed, 27 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/crypto/mediatek-crypto.txt

Acked-by: Rob Herring 

Re: [PATCH v3 2/2] crypto: mediatek - add DT bindings documentation

[Rob Herring]

On Mon, Dec 19, 2016 at 10:20:45AM +0800, Ryder Lee wrote:
> Add DT bindings documentation for the crypto driver
> 
> Signed-off-by: Ryder Lee 
> ---
>  .../devicetree/bindings/crypto/mediatek-crypto.txt | 27 
> ++
>  1 file changed, 27 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/crypto/mediatek-crypto.txt

Acked-by: Rob Herring 

Re: [PATCH 3/3] lib: Update LZ4 compressor module based on LZ4 v1.7.2.

[Sven Schmidt]

On 12/22/2016 06:31 PM, Greg KH wrote:
>On Wed, Dec 21, 2016 at 09:04:02PM +0100, Sven Schmidt wrote:
>> On 12/20/2016 08:52 PM, Joe Perches wrote:
>> > On Tue, 2016-12-20 at 19:53 +0100, Sven Schmidt wrote:
>> >> This patch updates LZ4 kernel module to LZ4 v1.7.2 by Yann Collet.
>> >> The kernel module is inspired by the previous work by Chanho Min.
>> >> The updated LZ4 module will not break existing code since there were alias
>> >> methods added to ensure backwards compatibility.
>> >
>> >[]
>> >
>> >> diff --git a/include/linux/lz4.h b/include/linux/lz4.h
>> > []
>> >> @@ -1,87 +1,218 @@
>> >>  #ifndef __LZ4_H__
>> >>  #define __LZ4_H__
>> >> +
>> >>  /*
>> >>   * LZ4 Kernel Interface
>> >>   *
>> >> - * Copyright (C) 2013, LG Electronics, Kyungsik Lee 
>> >> 
>> >> + * Copyright (C) 2016, Sven Schmidt <4ssch...@informatik.uni-hamburg.de>
>> >
>> > Deleting copyright notices is poor form and shouldn't be done.
>> >
>> > I didn't look at the rest
>> >
>>
>> Thanks Joe for your time. If you would take a look at the rest, you may 
>> notice
>> that I changed almost the whole file. That's why I also changed the 
>> copyright note.
>> If you think I should keep the original note, I will do that.
>
>You have to, please consult a lawyer if you have questions about
>copyright issues.  Or better yet, there's a free presentation/course
>online about copyright on the linuxfoundation.org website somewhere that
>should answer all of your questions here.  I recommend taking a few
>minutes and going through that.
>
>thanks,
>
>greg k-h
>

Will do, thanks Greg!

Re: [PATCH 3/3] lib: Update LZ4 compressor module based on LZ4 v1.7.2.

[Sven Schmidt]

On 12/22/2016 06:31 PM, Greg KH wrote:
>On Wed, Dec 21, 2016 at 09:04:02PM +0100, Sven Schmidt wrote:
>> On 12/20/2016 08:52 PM, Joe Perches wrote:
>> > On Tue, 2016-12-20 at 19:53 +0100, Sven Schmidt wrote:
>> >> This patch updates LZ4 kernel module to LZ4 v1.7.2 by Yann Collet.
>> >> The kernel module is inspired by the previous work by Chanho Min.
>> >> The updated LZ4 module will not break existing code since there were alias
>> >> methods added to ensure backwards compatibility.
>> >
>> >[]
>> >
>> >> diff --git a/include/linux/lz4.h b/include/linux/lz4.h
>> > []
>> >> @@ -1,87 +1,218 @@
>> >>  #ifndef __LZ4_H__
>> >>  #define __LZ4_H__
>> >> +
>> >>  /*
>> >>   * LZ4 Kernel Interface
>> >>   *
>> >> - * Copyright (C) 2013, LG Electronics, Kyungsik Lee 
>> >> 
>> >> + * Copyright (C) 2016, Sven Schmidt <4ssch...@informatik.uni-hamburg.de>
>> >
>> > Deleting copyright notices is poor form and shouldn't be done.
>> >
>> > I didn't look at the rest
>> >
>>
>> Thanks Joe for your time. If you would take a look at the rest, you may 
>> notice
>> that I changed almost the whole file. That's why I also changed the 
>> copyright note.
>> If you think I should keep the original note, I will do that.
>
>You have to, please consult a lawyer if you have questions about
>copyright issues.  Or better yet, there's a free presentation/course
>online about copyright on the linuxfoundation.org website somewhere that
>should answer all of your questions here.  I recommend taking a few
>minutes and going through that.
>
>thanks,
>
>greg k-h
>

Will do, thanks Greg!

Re: [PATCH v2 2/4] dt-bindings: add bindings for rk3328 clock controller

[Rob Herring]

On Mon, Dec 19, 2016 at 09:56:11AM +0800, Elaine Zhang wrote:
> Add devicetree bindings for Rockchip cru which found on
> Rockchip SoCs.
> 
> Signed-off-by: Elaine Zhang 
> ---
>  .../bindings/clock/rockchip,rk3328-cru.txt | 57 
> ++
>  1 file changed, 57 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt 
> b/Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt
> new file mode 100644
> index ..20053494d49f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt
> @@ -0,0 +1,57 @@
> +* Rockchip RK3328 Clock and Reset Unit
> +
> +The RK3328 clock controller generates and supplies clock to various
> +controllers within the SoC and also implements a reset controller for SoC
> +peripherals.
> +
> +Required Properties:
> +
> +- compatible: should be "rockchip,rk3328-cru"

The example shows other compatibles. IMO, I would drop them rather than 
add them here.

> +- reg: physical base address of the controller and length of memory mapped
> +  region.
> +- #clock-cells: should be 1.
> +- #reset-cells: should be 1.
> +
> +Optional Properties:
> +
> +- rockchip,grf: phandle to the syscon managing the "general register files"
> +  If missing pll rates are not changeable, due to the missing pll lock 
> status.
> +
> +Each clock is assigned an identifier and client nodes can use this identifier
> +to specify the clock which they consume. All available clocks are defined as
> +preprocessor macros in the dt-bindings/clock/rk3328-cru.h headers and can be
> +used in device tree sources. Similar macros exist for the reset sources in
> +these files.
> +
> +External clocks:
> +
> +There are several clocks that are generated outside the SoC. It is expected
> +that they are defined using standard clock bindings with following
> +clock-output-names:
> + - "xin24m" - crystal input - required,
> + - "clkin_i2s" - external I2S clock - optional,
> + - "gmac_clkin" - external GMAC clock - optional
> + - "phy_50m_out" - output clock of the pll in the mac phy
> +
> +Example: Clock controller node:
> +
> + cru: clock-controller@ff44 {
> + compatible = "rockchip,rk3328-cru", "rockchip,cru", "syscon";
> + reg = <0x0 0xff44 0x0 0x1000>;
> + rockchip,grf = <>;
> +
> + #clock-cells = <1>;
> + #reset-cells = <1>;
> + };
> +
> +Example: UART controller node that consumes the clock generated by the clock
> +  controller:
> +
> + uart0: serial@ff12 {
> + compatible = "snps,dw-apb-uart";
> + reg = <0xff12 0x100>;
> + interrupts = ;
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + clocks = < SCLK_UART0>;
> + };
> -- 
> 1.9.1
> 
> 

Re: [PATCH v2 2/4] dt-bindings: add bindings for rk3328 clock controller

[Rob Herring]

On Mon, Dec 19, 2016 at 09:56:11AM +0800, Elaine Zhang wrote:
> Add devicetree bindings for Rockchip cru which found on
> Rockchip SoCs.
> 
> Signed-off-by: Elaine Zhang 
> ---
>  .../bindings/clock/rockchip,rk3328-cru.txt | 57 
> ++
>  1 file changed, 57 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt 
> b/Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt
> new file mode 100644
> index ..20053494d49f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt
> @@ -0,0 +1,57 @@
> +* Rockchip RK3328 Clock and Reset Unit
> +
> +The RK3328 clock controller generates and supplies clock to various
> +controllers within the SoC and also implements a reset controller for SoC
> +peripherals.
> +
> +Required Properties:
> +
> +- compatible: should be "rockchip,rk3328-cru"

The example shows other compatibles. IMO, I would drop them rather than 
add them here.

> +- reg: physical base address of the controller and length of memory mapped
> +  region.
> +- #clock-cells: should be 1.
> +- #reset-cells: should be 1.
> +
> +Optional Properties:
> +
> +- rockchip,grf: phandle to the syscon managing the "general register files"
> +  If missing pll rates are not changeable, due to the missing pll lock 
> status.
> +
> +Each clock is assigned an identifier and client nodes can use this identifier
> +to specify the clock which they consume. All available clocks are defined as
> +preprocessor macros in the dt-bindings/clock/rk3328-cru.h headers and can be
> +used in device tree sources. Similar macros exist for the reset sources in
> +these files.
> +
> +External clocks:
> +
> +There are several clocks that are generated outside the SoC. It is expected
> +that they are defined using standard clock bindings with following
> +clock-output-names:
> + - "xin24m" - crystal input - required,
> + - "clkin_i2s" - external I2S clock - optional,
> + - "gmac_clkin" - external GMAC clock - optional
> + - "phy_50m_out" - output clock of the pll in the mac phy
> +
> +Example: Clock controller node:
> +
> + cru: clock-controller@ff44 {
> + compatible = "rockchip,rk3328-cru", "rockchip,cru", "syscon";
> + reg = <0x0 0xff44 0x0 0x1000>;
> + rockchip,grf = <>;
> +
> + #clock-cells = <1>;
> + #reset-cells = <1>;
> + };
> +
> +Example: UART controller node that consumes the clock generated by the clock
> +  controller:
> +
> + uart0: serial@ff12 {
> + compatible = "snps,dw-apb-uart";
> + reg = <0xff12 0x100>;
> + interrupts = ;
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + clocks = < SCLK_UART0>;
> + };
> -- 
> 1.9.1
> 
> 

Re: [PATCH] dax: kill uml support

[Dan Williams]

On Thu, Dec 22, 2016 at 8:40 AM, Ross Zwisler
 wrote:
> On Thu, Dec 22, 2016 at 11:00:55AM +0100, Jan Kara wrote:
>> On Wed 21-12-16 10:51:18, Ross Zwisler wrote:
>> > On Wed, Dec 21, 2016 at 09:53:47AM +0100, Jan Kara wrote:
>> > > On Tue 20-12-16 17:37:40, Dan Williams wrote:
>> > > > The lack of common transparent-huge-page helpers for UML is becoming
>> > > > increasingly painful for fs/dax.c now that it is growing more pmd
>> > > > functionality. Add UML to the list of unsupported architectures, and
>> > > > clean up no-longer-necessary ifdef as a result.
>> > > ...
>> > > > diff --git a/fs/dax.c b/fs/dax.c
>> > > > index ddcddfeaa03b..86df835783ea 100644
>> > > > --- a/fs/dax.c
>> > > > +++ b/fs/dax.c
>> > > > @@ -710,8 +710,7 @@ static void dax_mapping_entry_mkclean(struct 
>> > > > address_space *mapping,
>> > > > if (follow_pte_pmd(vma->vm_mm, address, , , 
>> > > > ))
>> > > > continue;
>> > > >
>> > > > -   if (pmdp) {
>> > > > -#ifdef CONFIG_FS_DAX_PMD
>> > > > +   if (pmdp && IS_ENABLED(CONFIG_FS_DAX_PMD)) {
>> > > > pmd_t pmd;
>> > >
>> > > So I was under the impression that pmdp can never be != NULL when
>> > > CONFIG_FS_DAX_PMD is disabled. Otherwise Ross' patch would leave ptl 
>> > > locked
>> > > in that case... Did I miss something or we can just remove IS_ENABLED()
>> > > check?
>> >
>> > We need the IS_ENABLED() check to prevent a different build error.
>> >
>> > The #ifdef was there to prevent compile time errors where pmd_pfn(),
>> > pmd_write(), etc were all undefined symbols because they aren't defined in
>> > the arch/um headers.
>> >
>> > For x86_64 we can get linker errors if CONFIG_TRANSPARENT_HUGEPAGE isn't 
>> > set:
>> >
>> > fs/built-in.o: In function `dax_writeback_mapping_range.part.27':
>> > dax.c:(.text+0x4ce28): undefined reference to `pmdp_huge_clear_flush'
>> >
>> > In this config we do have a prototype for pmdp_huge_clear_flush() in
>> > include/asm-generic/pgtable.h so it doesn't show up as an undefined symbol,
>> > but the implementation in mm/pgtable-generic.c is in a
>> > #ifdef CONFIG_TRANSPARENT_HUGEPAGE block.
>> >
>> > The IS_ENABLED() lets the compiler optimize out the code that calls
>> > pmdp_huge_clear_flush() so it doesn't try and resolve that symbol at link
>> > time.
>>
>> Ah, OK. Won't it be cleaner to just have empty stub for
>> pmdp_huge_clear_flush() if !CONFIG_TRANSPARENT_HUGEPAGE? I like that more
>> but I can live with IS_ENABLED check as well but please add a comment that
>> it is there only to make compiler happy.
>
> Yea, making an empty stub is cleaner.  That has the added bonus that we don't
> have to rely on the assumption that 'pmdp' will always be NULL in if
> CONFIG_TRANSPARENT_HUGEPAGE isn't set.
>
> I'll reflow & send out v2.

If you're going deeper might as well look at killing the other ifdef
CONFIG_FS_DAX_PMD in that file as well.

Re: [PATCH] dax: kill uml support

[Dan Williams]

On Thu, Dec 22, 2016 at 8:40 AM, Ross Zwisler
 wrote:
> On Thu, Dec 22, 2016 at 11:00:55AM +0100, Jan Kara wrote:
>> On Wed 21-12-16 10:51:18, Ross Zwisler wrote:
>> > On Wed, Dec 21, 2016 at 09:53:47AM +0100, Jan Kara wrote:
>> > > On Tue 20-12-16 17:37:40, Dan Williams wrote:
>> > > > The lack of common transparent-huge-page helpers for UML is becoming
>> > > > increasingly painful for fs/dax.c now that it is growing more pmd
>> > > > functionality. Add UML to the list of unsupported architectures, and
>> > > > clean up no-longer-necessary ifdef as a result.
>> > > ...
>> > > > diff --git a/fs/dax.c b/fs/dax.c
>> > > > index ddcddfeaa03b..86df835783ea 100644
>> > > > --- a/fs/dax.c
>> > > > +++ b/fs/dax.c
>> > > > @@ -710,8 +710,7 @@ static void dax_mapping_entry_mkclean(struct 
>> > > > address_space *mapping,
>> > > > if (follow_pte_pmd(vma->vm_mm, address, , , 
>> > > > ))
>> > > > continue;
>> > > >
>> > > > -   if (pmdp) {
>> > > > -#ifdef CONFIG_FS_DAX_PMD
>> > > > +   if (pmdp && IS_ENABLED(CONFIG_FS_DAX_PMD)) {
>> > > > pmd_t pmd;
>> > >
>> > > So I was under the impression that pmdp can never be != NULL when
>> > > CONFIG_FS_DAX_PMD is disabled. Otherwise Ross' patch would leave ptl 
>> > > locked
>> > > in that case... Did I miss something or we can just remove IS_ENABLED()
>> > > check?
>> >
>> > We need the IS_ENABLED() check to prevent a different build error.
>> >
>> > The #ifdef was there to prevent compile time errors where pmd_pfn(),
>> > pmd_write(), etc were all undefined symbols because they aren't defined in
>> > the arch/um headers.
>> >
>> > For x86_64 we can get linker errors if CONFIG_TRANSPARENT_HUGEPAGE isn't 
>> > set:
>> >
>> > fs/built-in.o: In function `dax_writeback_mapping_range.part.27':
>> > dax.c:(.text+0x4ce28): undefined reference to `pmdp_huge_clear_flush'
>> >
>> > In this config we do have a prototype for pmdp_huge_clear_flush() in
>> > include/asm-generic/pgtable.h so it doesn't show up as an undefined symbol,
>> > but the implementation in mm/pgtable-generic.c is in a
>> > #ifdef CONFIG_TRANSPARENT_HUGEPAGE block.
>> >
>> > The IS_ENABLED() lets the compiler optimize out the code that calls
>> > pmdp_huge_clear_flush() so it doesn't try and resolve that symbol at link
>> > time.
>>
>> Ah, OK. Won't it be cleaner to just have empty stub for
>> pmdp_huge_clear_flush() if !CONFIG_TRANSPARENT_HUGEPAGE? I like that more
>> but I can live with IS_ENABLED check as well but please add a comment that
>> it is there only to make compiler happy.
>
> Yea, making an empty stub is cleaner.  That has the added bonus that we don't
> have to rely on the assumption that 'pmdp' will always be NULL in if
> CONFIG_TRANSPARENT_HUGEPAGE isn't set.
>
> I'll reflow & send out v2.

If you're going deeper might as well look at killing the other ifdef
CONFIG_FS_DAX_PMD in that file as well.

Re: [PATCH 0/3] Update LZ4 compressor module

[Sven Schmidt]

On 12/22/2016 06:29 PM, Greg KH wrote:
> On Tue, Dec 20, 2016 at 07:53:09PM +0100, Sven Schmidt wrote:
>>
>> This patchset is for updating the LZ4 compression module to a version based
>> on LZ4 v1.7.2 allowing to use the fast compression algorithm aka LZ4 fast
>> which provides an "acceleration" parameter as a tradeoff between
>> compression ratio and compression speed.
>
> But why do this?
>
>> We will use LZ4 fast in order to support compression in lustre. LZ4 fast 
>> empowers
>> us to do client-side as well as server-side compression/decompression while
>> being able to provide appropriate parameters to enable users to tune lustre's
>> behaviour to obtain the best performance/compression/etc. on their behalf
>> (adapative compression).
>
> We don't care about lustre, especially as it is not merged into the main
> portion of the kernel tree :)
>
> Seriously, work on fixing up the known issues in lustre before adding
> additional features, I've only been saying this for a few _years_ now...
>

Hey Greg and thanks for your time. Actually, we're not the guys behind lustre,
hence I'd leave fixing the bugs in lustre to them ;)

I'm working with the research group for scientific computing on the University
of Hamburg on the German Climate Computing Centre. The research group is working
with high performance storage systems etc. In this case, we investigate
data reduction techniques in behalf of the increasing gap between computational 
speed,
network speed and storage capacity. That's why we're ultimately aiming
for compression support in lustre.

Initial studies have shown that an adequate compression ratio for scientific 
data
can be archieved using LZ4. Since the currently available version of LZ4
in the kernel is about three years old, we would love if you accept 
our work on getting a more current version into the kernel.

>> Also, it will be useful for other users of LZ4 compression,
>> as with LZ4 fast it is possible to enable applications to use fast and/or 
>> high
>> compression depending of the usecase. E.g. a developer can use very
>> high compression (low acceleration) for sending data over a network with
>> limited rate of transmission or he trades the compression ratio for higher
>> compression speed.
>
> This whole patch series is broken, always test-build your code, there's
> nothing we could do with these patches even if we wanted to :(
>
> greg k-h
>

I'm sorry, this is my first patchset, so please be kind :(
Already got rid of the issues on my local machine and reviewed the output from
the buildbots. Will send an updated patchset later!

Thanks
Sven

Re: [PATCH 0/3] Update LZ4 compressor module

[Sven Schmidt]

On 12/22/2016 06:29 PM, Greg KH wrote:
> On Tue, Dec 20, 2016 at 07:53:09PM +0100, Sven Schmidt wrote:
>>
>> This patchset is for updating the LZ4 compression module to a version based
>> on LZ4 v1.7.2 allowing to use the fast compression algorithm aka LZ4 fast
>> which provides an "acceleration" parameter as a tradeoff between
>> compression ratio and compression speed.
>
> But why do this?
>
>> We will use LZ4 fast in order to support compression in lustre. LZ4 fast 
>> empowers
>> us to do client-side as well as server-side compression/decompression while
>> being able to provide appropriate parameters to enable users to tune lustre's
>> behaviour to obtain the best performance/compression/etc. on their behalf
>> (adapative compression).
>
> We don't care about lustre, especially as it is not merged into the main
> portion of the kernel tree :)
>
> Seriously, work on fixing up the known issues in lustre before adding
> additional features, I've only been saying this for a few _years_ now...
>

Hey Greg and thanks for your time. Actually, we're not the guys behind lustre,
hence I'd leave fixing the bugs in lustre to them ;)

I'm working with the research group for scientific computing on the University
of Hamburg on the German Climate Computing Centre. The research group is working
with high performance storage systems etc. In this case, we investigate
data reduction techniques in behalf of the increasing gap between computational 
speed,
network speed and storage capacity. That's why we're ultimately aiming
for compression support in lustre.

Initial studies have shown that an adequate compression ratio for scientific 
data
can be archieved using LZ4. Since the currently available version of LZ4
in the kernel is about three years old, we would love if you accept 
our work on getting a more current version into the kernel.

>> Also, it will be useful for other users of LZ4 compression,
>> as with LZ4 fast it is possible to enable applications to use fast and/or 
>> high
>> compression depending of the usecase. E.g. a developer can use very
>> high compression (low acceleration) for sending data over a network with
>> limited rate of transmission or he trades the compression ratio for higher
>> compression speed.
>
> This whole patch series is broken, always test-build your code, there's
> nothing we could do with these patches even if we wanted to :(
>
> greg k-h
>

I'm sorry, this is my first patchset, so please be kind :(
Already got rid of the issues on my local machine and reviewed the output from
the buildbots. Will send an updated patchset later!

Thanks
Sven

Re: [PATCH v3 13/15] livepatch: change to a per-task consistency model

[Josh Poimboeuf]

On Thu, Dec 22, 2016 at 03:34:52PM +0100, Petr Mladek wrote:
> On Wed 2016-12-21 15:25:05, Josh Poimboeuf wrote:
> > On Tue, Dec 20, 2016 at 06:32:46PM +0100, Petr Mladek wrote:
> > > On Thu 2016-12-08 12:08:38, Josh Poimboeuf wrote:
> > > > Change livepatch to use a basic per-task consistency model.  This is the
> > > > foundation which will eventually enable us to patch those ~10% of
> > > > security patches which change function or data semantics.  This is the
> > > > biggest remaining piece needed to make livepatch more generally useful.
> > > > 
> > > > [1] https://lkml.kernel.org/r/20141107140458.ga21...@suse.cz
> > > > 
> > > > --- /dev/null
> > > > +++ b/kernel/livepatch/transition.c
> > > > +/*
> > > > + * Initialize the global target patch state and all tasks to the 
> > > > initial patch
> > > > + * state, and initialize all function transition states to true in 
> > > > preparation
> > > > + * for patching or unpatching.
> > > > + */
> > > > +void klp_init_transition(struct klp_patch *patch, int state)
> > > > +{
> > > > +   struct task_struct *g, *task;
> > > > +   unsigned int cpu;
> > > > +   struct klp_object *obj;
> > > > +   struct klp_func *func;
> > > > +   int initial_state = !state;
> > > > +
> > > > +   WARN_ON_ONCE(klp_target_state != KLP_UNDEFINED);
> > > > +
> > > > +   klp_transition_patch = patch;
> > > > +
> > > > +   /*
> > > > +* Set the global target patch state which tasks will switch 
> > > > to.  This
> > > > +* has no effect until the TIF_PATCH_PENDING flags get set 
> > > > later.
> > > > +*/
> > > > +   klp_target_state = state;
> > > > +
> > > > +   /*
> > > > +* If the patch can be applied or reverted immediately, skip the
> > > > +* per-task transitions.
> > > > +*/
> > > > +   if (patch->immediate)
> > > > +   return;
> > > > +
> > > > +   /*
> > > > +* Initialize all tasks to the initial patch state to prepare 
> > > > them for
> > > > +* switching to the target state.
> > > > +*/
> > > > +   read_lock(_lock);
> > > > +   for_each_process_thread(g, task) {
> > > > +   WARN_ON_ONCE(task->patch_state != KLP_UNDEFINED);
> > > > +   task->patch_state = initial_state;
> > > > +   }
> > > > +   read_unlock(_lock);
> > > > +
> > > > +   /*
> > > > +* Ditto for the idle "swapper" tasks.
> > > > +*/
> > > > +   get_online_cpus();
> > > > +   for_each_online_cpu(cpu) {
> > > > +   task = idle_task(cpu);
> > > > +   WARN_ON_ONCE(task->patch_state != KLP_UNDEFINED);
> > > > +   task->patch_state = initial_state;
> > > > +   }
> > > > +   put_online_cpus();
> > > 
> > > We allow to add/remove CPUs here. I am afraid that we will also need
> > > to add a cpu coming/going handler that will set the task->patch_state
> > > the right way. We must not set the klp_target_state until all ftrace
> > > handlers are ready.
> > 
> > What if we instead just change the above to use for_each_possible_cpu()?
> > We could do the same in klp_complete_transition().
> 
> I like this idea. It seems that there is idle task for each possible
> cpu, see idle_threads_init().
> 
> IMHO, we should do the same everytime we do anything with the idle
> tasks. I mean in klp_start_transition, klp_try_complete_transition()
> and also complete_transition().
> 
> Then they will be handled like any other processes and we do not need
> to think of any special races.

More on this below.

> > > > +   /*
> > > > +* Enforce the order of the task->patch_state initializations 
> > > > and the
> > > > +* func->transition updates to ensure that, in the enable path,
> > > > +* klp_ftrace_handler() doesn't see a func in transition with a
> > > > +* task->patch_state of KLP_UNDEFINED.
> > > > +*/
> > > > +   smp_wmb();
> > > > +
> > > > +   /*
> > > > +* Set the func transition states so klp_ftrace_handler() will 
> > > > know to
> > > > +* switch to the transition logic.
> > > > +*
> > > > +* When patching, the funcs aren't yet in the func_stack and 
> > > > will be
> > > > +* made visible to the ftrace handler shortly by the calls to
> > > > +* klp_patch_object().
> > > > +*
> > > > +* When unpatching, the funcs are already in the func_stack and 
> > > > so are
> > > > +* already visible to the ftrace handler.
> > > > +*/
> > > > +   klp_for_each_object(patch, obj)
> > > > +   klp_for_each_func(obj, func)
> > > > +   func->transition = true;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Start the transition to the specified target patch state so tasks 
> > > > can begin
> > > > + * switching to it.
> > > > + */
> > > > +void klp_start_transition(void)
> > > > +{
> > > > +   struct task_struct *g, *task;
> > > > +  

Re: [PATCH v3 13/15] livepatch: change to a per-task consistency model

[Josh Poimboeuf]

On Thu, Dec 22, 2016 at 03:34:52PM +0100, Petr Mladek wrote:
> On Wed 2016-12-21 15:25:05, Josh Poimboeuf wrote:
> > On Tue, Dec 20, 2016 at 06:32:46PM +0100, Petr Mladek wrote:
> > > On Thu 2016-12-08 12:08:38, Josh Poimboeuf wrote:
> > > > Change livepatch to use a basic per-task consistency model.  This is the
> > > > foundation which will eventually enable us to patch those ~10% of
> > > > security patches which change function or data semantics.  This is the
> > > > biggest remaining piece needed to make livepatch more generally useful.
> > > > 
> > > > [1] https://lkml.kernel.org/r/20141107140458.ga21...@suse.cz
> > > > 
> > > > --- /dev/null
> > > > +++ b/kernel/livepatch/transition.c
> > > > +/*
> > > > + * Initialize the global target patch state and all tasks to the 
> > > > initial patch
> > > > + * state, and initialize all function transition states to true in 
> > > > preparation
> > > > + * for patching or unpatching.
> > > > + */
> > > > +void klp_init_transition(struct klp_patch *patch, int state)
> > > > +{
> > > > +   struct task_struct *g, *task;
> > > > +   unsigned int cpu;
> > > > +   struct klp_object *obj;
> > > > +   struct klp_func *func;
> > > > +   int initial_state = !state;
> > > > +
> > > > +   WARN_ON_ONCE(klp_target_state != KLP_UNDEFINED);
> > > > +
> > > > +   klp_transition_patch = patch;
> > > > +
> > > > +   /*
> > > > +* Set the global target patch state which tasks will switch 
> > > > to.  This
> > > > +* has no effect until the TIF_PATCH_PENDING flags get set 
> > > > later.
> > > > +*/
> > > > +   klp_target_state = state;
> > > > +
> > > > +   /*
> > > > +* If the patch can be applied or reverted immediately, skip the
> > > > +* per-task transitions.
> > > > +*/
> > > > +   if (patch->immediate)
> > > > +   return;
> > > > +
> > > > +   /*
> > > > +* Initialize all tasks to the initial patch state to prepare 
> > > > them for
> > > > +* switching to the target state.
> > > > +*/
> > > > +   read_lock(_lock);
> > > > +   for_each_process_thread(g, task) {
> > > > +   WARN_ON_ONCE(task->patch_state != KLP_UNDEFINED);
> > > > +   task->patch_state = initial_state;
> > > > +   }
> > > > +   read_unlock(_lock);
> > > > +
> > > > +   /*
> > > > +* Ditto for the idle "swapper" tasks.
> > > > +*/
> > > > +   get_online_cpus();
> > > > +   for_each_online_cpu(cpu) {
> > > > +   task = idle_task(cpu);
> > > > +   WARN_ON_ONCE(task->patch_state != KLP_UNDEFINED);
> > > > +   task->patch_state = initial_state;
> > > > +   }
> > > > +   put_online_cpus();
> > > 
> > > We allow to add/remove CPUs here. I am afraid that we will also need
> > > to add a cpu coming/going handler that will set the task->patch_state
> > > the right way. We must not set the klp_target_state until all ftrace
> > > handlers are ready.
> > 
> > What if we instead just change the above to use for_each_possible_cpu()?
> > We could do the same in klp_complete_transition().
> 
> I like this idea. It seems that there is idle task for each possible
> cpu, see idle_threads_init().
> 
> IMHO, we should do the same everytime we do anything with the idle
> tasks. I mean in klp_start_transition, klp_try_complete_transition()
> and also complete_transition().
> 
> Then they will be handled like any other processes and we do not need
> to think of any special races.

More on this below.

> > > > +   /*
> > > > +* Enforce the order of the task->patch_state initializations 
> > > > and the
> > > > +* func->transition updates to ensure that, in the enable path,
> > > > +* klp_ftrace_handler() doesn't see a func in transition with a
> > > > +* task->patch_state of KLP_UNDEFINED.
> > > > +*/
> > > > +   smp_wmb();
> > > > +
> > > > +   /*
> > > > +* Set the func transition states so klp_ftrace_handler() will 
> > > > know to
> > > > +* switch to the transition logic.
> > > > +*
> > > > +* When patching, the funcs aren't yet in the func_stack and 
> > > > will be
> > > > +* made visible to the ftrace handler shortly by the calls to
> > > > +* klp_patch_object().
> > > > +*
> > > > +* When unpatching, the funcs are already in the func_stack and 
> > > > so are
> > > > +* already visible to the ftrace handler.
> > > > +*/
> > > > +   klp_for_each_object(patch, obj)
> > > > +   klp_for_each_func(obj, func)
> > > > +   func->transition = true;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Start the transition to the specified target patch state so tasks 
> > > > can begin
> > > > + * switching to it.
> > > > + */
> > > > +void klp_start_transition(void)
> > > > +{
> > > > +   struct task_struct *g, *task;
> > > > +  

Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

[Stephen Boyd]

On 12/21/2016 05:07 PM, Pierre-Louis Bossart wrote:
> On 12/21/16 5:05 PM, Stephen Boyd wrote:
>>
>> Ok, by clkdev design if a device is passed but there isn't a
>> match in the lookup table it allows it to match based solely on
>> the connection id. Given that the connection id is globally
>> unique this will work.
>>
>> Hopefully we don't have two of these devices with pmc_plt_clk_
>> signals in a single system though. Then having the device name
>> would help differentiate between the two. And then it may make
>> sense to have some sort of ACPI lookup system, similar to how we
>> have lookups for clks in DT.
>
> So in short we keep the existing solution for now and will only use
> the device name if and when the pmc_plt_clk_ identifier is no
> longer unique due to hardware changes. Did I get this right?

Ok.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

[Stephen Boyd]

On 12/21/2016 05:07 PM, Pierre-Louis Bossart wrote:
> On 12/21/16 5:05 PM, Stephen Boyd wrote:
>>
>> Ok, by clkdev design if a device is passed but there isn't a
>> match in the lookup table it allows it to match based solely on
>> the connection id. Given that the connection id is globally
>> unique this will work.
>>
>> Hopefully we don't have two of these devices with pmc_plt_clk_
>> signals in a single system though. Then having the device name
>> would help differentiate between the two. And then it may make
>> sense to have some sort of ACPI lookup system, similar to how we
>> have lookups for clks in DT.
>
> So in short we keep the existing solution for now and will only use
> the device name if and when the pmc_plt_clk_ identifier is no
> longer unique due to hardware changes. Did I get this right?

Ok.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: "klist" "k" stand for what ?

[Randy Dunlap]

On 12/21/16 02:23, galcon zhao wrote:
>  HI ALL:
> 
> I am reading kernel source code ,but  I have a question.
> 
> struct klist {
> spinlock_t k_lock;
> struct list_head k_list;
> void (*get)(struct klist_node *);
> void (*put)(struct klist_node *);
> } __attribute__ ((aligned (sizeof(void *;
> "klist""k"  stand for what  ?
> 
> I think "k" is "key",  Is it right?

Not sure, but I expect that it means "kernel" list.

-- 
~Randy

Re: "klist" "k" stand for what ?

[Randy Dunlap]

On 12/21/16 02:23, galcon zhao wrote:
>  HI ALL:
> 
> I am reading kernel source code ,but  I have a question.
> 
> struct klist {
> spinlock_t k_lock;
> struct list_head k_list;
> void (*get)(struct klist_node *);
> void (*put)(struct klist_node *);
> } __attribute__ ((aligned (sizeof(void *;
> "klist""k"  stand for what  ?
> 
> I think "k" is "key",  Is it right?

Not sure, but I expect that it means "kernel" list.

-- 
~Randy

Re: [PATCH v4 1/3] perf: add PERF_RECORD_NAMESPACES to include namespaces related info

[Eric W. Biederman]

Peter Zijlstra  writes:

> On Thu, Dec 22, 2016 at 11:19:17PM +1300, Eric W. Biederman wrote:
>> Peter Zijlstra  writes:
>> 
>> > On Thu, Dec 22, 2016 at 08:21:23PM +1300, Eric W. Biederman wrote:
>> >> 
>> >> And please make the array the last item in the structure so that
>> >> expanding or contracting it does not affect the ability to read the rest
>> >> of the structure.
>> >
>> > Sorry, sample_id must be last, because hysterical crud :/
>> >
>> > (basically because that was the only way to add a field to records like
>> > PERF_RECORD_MMAP which used the record length to determine the
>> > filename[] length, yes I know, we won't ever do that again).
>> 
>> Why does historical crud need to affect new records?
>
> Because now the userspace parser expects sample_id to be the tail
> field. Basically decoding a record now looks like:
>
>   if (sample_id_all) {
>   sample_id = (sample_id *)((char *)record + record->size - 
> sizeof(sample_id));
>   record->size -= sizeof(sample_id);
>   }
>   /* process record */
>
> We could of course create more exceptions..
>
>> Totally confused.  This looks like a major mess.
>
> Not major, but yes, its ugly, but its also ABI :-(

Fair enough.  I will just avert my eyes now and deal with my own challenges.

Eric

Re: [PATCH v4 1/3] perf: add PERF_RECORD_NAMESPACES to include namespaces related info

[Eric W. Biederman]

Peter Zijlstra  writes:

> On Thu, Dec 22, 2016 at 11:19:17PM +1300, Eric W. Biederman wrote:
>> Peter Zijlstra  writes:
>> 
>> > On Thu, Dec 22, 2016 at 08:21:23PM +1300, Eric W. Biederman wrote:
>> >> 
>> >> And please make the array the last item in the structure so that
>> >> expanding or contracting it does not affect the ability to read the rest
>> >> of the structure.
>> >
>> > Sorry, sample_id must be last, because hysterical crud :/
>> >
>> > (basically because that was the only way to add a field to records like
>> > PERF_RECORD_MMAP which used the record length to determine the
>> > filename[] length, yes I know, we won't ever do that again).
>> 
>> Why does historical crud need to affect new records?
>
> Because now the userspace parser expects sample_id to be the tail
> field. Basically decoding a record now looks like:
>
>   if (sample_id_all) {
>   sample_id = (sample_id *)((char *)record + record->size - 
> sizeof(sample_id));
>   record->size -= sizeof(sample_id);
>   }
>   /* process record */
>
> We could of course create more exceptions..
>
>> Totally confused.  This looks like a major mess.
>
> Not major, but yes, its ugly, but its also ABI :-(

Fair enough.  I will just avert my eyes now and deal with my own challenges.

Eric

Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time

[Guenter Roeck]

On Thu, Dec 22, 2016 at 04:45:45PM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 22 Dec 2016, Guenter Roeck wrote:
> 
> > On 12/22/2016 04:29 AM, Julia Lawall wrote:
> > >
> > >
> > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > >
> > > > Hi Julia,
> > > >
> > > > On Wed, Dec 21, 2016 at 08:39:38PM +0100, Julia Lawall wrote:
> > > > >
> > > > >
> > > > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > > > >
> > > > > > Hi Julia,
> > > > > >
> > > > > > On Wed, Dec 21, 2016 at 03:05:37PM +0100, Julia Lawall wrote:
> > > > > > > A solution is below: the semantic patch, an explanation of the
> > > > > > > semantic
> > > > > > > patch, and the results.  I have only tried to compile the results
> > > > > > > (make
> > > > > > > drivers/hwmon/).  Two affected files were not considered for
> > > > > > > compilation:
> > > > > > >
> > > > > > > drivers/hwmon/vexpress-hwmon.o
> > > > > > > drivers/hwmon/jz4740-hwmon.o
> > > >
> > > > I compile tested those two patches. If possible please drop
> > > > vexpress-hwmon.c
> > > > from the patch series; the changes in that file don't add any value.
> > > >
> > > > I compile tested all files, and reviewed the patch. It all looks good.
> > > > Please submit the series.
> > > >
> > > > Again, thanks a lot for your help!
> > >
> > > I have sent the patches.  I adjusted the semantic patch so that the
> > > indentation of function parameters/arguments would only change if the
> > > length of the function name changes.
> > >
> > > Do you think this could be of more general interest in the Linux kernel?
> > > Since the semantic patch works pretty well, I could add it to the
> > > scripts/coccinelle directory?  Previously, however, I got some negative
> > > feedback about this change, because people felt that the new names hid the
> > > actual behavior, so I didn't pursue it.
> > >
> >
> > I do think it would add a lot of value, if for nothing else as an excellent
> > example
> > of what can be done with coccinelle.
> >
> > I actually liked the name changes. I think it is a good idea if the function
> > name
> > reflects the sysfs attribute it serves (isn't that exactly what it does, ie
> > its
> > behavior ?). But, as you have experienced, some people inadvertently did not
> > like
> > it. Given that, I am not sure if it is worth adding it to the kernel source
> > tree.
> > Maybe you could submit it as RFC so it is at least on record.
> >
> > Anyway, for SENSOR_DEVICE_ATTR(), I'll have to be a bit more flexible since
> > the function _will_ be reused. I'll need something like
> > SENSOR_DEVICE_ATTR_{RO,RW,WO}(attr, func, param)
> 
> Chosen at random,
> 
> static SENSOR_DEVICE_ATTR_2(sf2_point4_fan1, S_IRUGO | S_IWUSR,
> show_sf2_point, store_sf2_point, 4, 1);
> 
> should become
> 
> static SENSOR_DEVICE_ATTR_2_RW(sf2_point4_fan1, sf2_point, 4, 1); ?
> 
> And the functions should be renamed with show and store at the end?
> 
Yes, exactly.  Of course, not all of them use "show_" at the beginning.
get_ and set_ are used as well. Essentially I would want to replace 
[driver_prefix]{show_,get_,set_}func{_get,_show,_set}
with 'func'.

If you have an idea how to do that, any hints would be welcome.

> > Maybe Greg would be open to something like
> > DEVICE_ATTR_FUNC_{RO,RW,WO}(attr,func)
> > to accommodate the "I want my own function name" crowd ? That would also 
> > solve
> > the case where the function is reused for multiple attributes.
> 
> Actually, it was the DEVICE_ATTR_{RO,RW,WO} that wasn't liked.  It doesn't
> show the exact permission numbers.  The fact that not all DEVICE_ATTR uses


It should be obvious that using the {RO,RW,WO} variants is less error prone.
Can anyone seriously argue against that ?

> can be changed due to function reuse is awkward, though.  Greg, do you
> have any thoughts about that?
> 
> Currently, there are around 1100 calls to DEVICE_ATTR_{RO,RW,WO}.
> 

If the problem is that people need to see exact permission numbers instead
of "RO" to understand that an attribute is read only, I think the semantic
patch should really be added to the kernel.

Thanks,
Guenter

Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time

[Guenter Roeck]

On Thu, Dec 22, 2016 at 04:45:45PM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 22 Dec 2016, Guenter Roeck wrote:
> 
> > On 12/22/2016 04:29 AM, Julia Lawall wrote:
> > >
> > >
> > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > >
> > > > Hi Julia,
> > > >
> > > > On Wed, Dec 21, 2016 at 08:39:38PM +0100, Julia Lawall wrote:
> > > > >
> > > > >
> > > > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > > > >
> > > > > > Hi Julia,
> > > > > >
> > > > > > On Wed, Dec 21, 2016 at 03:05:37PM +0100, Julia Lawall wrote:
> > > > > > > A solution is below: the semantic patch, an explanation of the
> > > > > > > semantic
> > > > > > > patch, and the results.  I have only tried to compile the results
> > > > > > > (make
> > > > > > > drivers/hwmon/).  Two affected files were not considered for
> > > > > > > compilation:
> > > > > > >
> > > > > > > drivers/hwmon/vexpress-hwmon.o
> > > > > > > drivers/hwmon/jz4740-hwmon.o
> > > >
> > > > I compile tested those two patches. If possible please drop
> > > > vexpress-hwmon.c
> > > > from the patch series; the changes in that file don't add any value.
> > > >
> > > > I compile tested all files, and reviewed the patch. It all looks good.
> > > > Please submit the series.
> > > >
> > > > Again, thanks a lot for your help!
> > >
> > > I have sent the patches.  I adjusted the semantic patch so that the
> > > indentation of function parameters/arguments would only change if the
> > > length of the function name changes.
> > >
> > > Do you think this could be of more general interest in the Linux kernel?
> > > Since the semantic patch works pretty well, I could add it to the
> > > scripts/coccinelle directory?  Previously, however, I got some negative
> > > feedback about this change, because people felt that the new names hid the
> > > actual behavior, so I didn't pursue it.
> > >
> >
> > I do think it would add a lot of value, if for nothing else as an excellent
> > example
> > of what can be done with coccinelle.
> >
> > I actually liked the name changes. I think it is a good idea if the function
> > name
> > reflects the sysfs attribute it serves (isn't that exactly what it does, ie
> > its
> > behavior ?). But, as you have experienced, some people inadvertently did not
> > like
> > it. Given that, I am not sure if it is worth adding it to the kernel source
> > tree.
> > Maybe you could submit it as RFC so it is at least on record.
> >
> > Anyway, for SENSOR_DEVICE_ATTR(), I'll have to be a bit more flexible since
> > the function _will_ be reused. I'll need something like
> > SENSOR_DEVICE_ATTR_{RO,RW,WO}(attr, func, param)
> 
> Chosen at random,
> 
> static SENSOR_DEVICE_ATTR_2(sf2_point4_fan1, S_IRUGO | S_IWUSR,
> show_sf2_point, store_sf2_point, 4, 1);
> 
> should become
> 
> static SENSOR_DEVICE_ATTR_2_RW(sf2_point4_fan1, sf2_point, 4, 1); ?
> 
> And the functions should be renamed with show and store at the end?
> 
Yes, exactly.  Of course, not all of them use "show_" at the beginning.
get_ and set_ are used as well. Essentially I would want to replace 
[driver_prefix]{show_,get_,set_}func{_get,_show,_set}
with 'func'.

If you have an idea how to do that, any hints would be welcome.

> > Maybe Greg would be open to something like
> > DEVICE_ATTR_FUNC_{RO,RW,WO}(attr,func)
> > to accommodate the "I want my own function name" crowd ? That would also 
> > solve
> > the case where the function is reused for multiple attributes.
> 
> Actually, it was the DEVICE_ATTR_{RO,RW,WO} that wasn't liked.  It doesn't
> show the exact permission numbers.  The fact that not all DEVICE_ATTR uses


It should be obvious that using the {RO,RW,WO} variants is less error prone.
Can anyone seriously argue against that ?

> can be changed due to function reuse is awkward, though.  Greg, do you
> have any thoughts about that?
> 
> Currently, there are around 1100 calls to DEVICE_ATTR_{RO,RW,WO}.
> 

If the problem is that people need to see exact permission numbers instead
of "RO" to understand that an attribute is read only, I think the semantic
patch should really be added to the kernel.

Thanks,
Guenter

Re: BPF hash algo (Re: [kernel-hardening] Re: [PATCH v7 3/6] random: use SipHash in place of MD5)

[Jason A. Donenfeld]

On Thu, Dec 22, 2016 at 5:59 PM, Hannes Frederic Sowa
 wrote:
> We don't prevent ebpf programs being loaded based on the digest but
> just to uniquely identify loaded programs from user space and match up
> with their source.

Okay, so in that case, a weak hashing function like SHA1 could result
in a real vulnerability. Therefore, this SHA1 stuff needs to be
reverted immediately, pending a different implementation. If this has
ever shipped in a kernel version, it could even deserve a CVE. No
SHA1!

> The hashing is not a proper sha1 neither, unfortunately. I think that
> is why it will have a custom implementation in iproute2?

Jeepers creepers. So for some ungodly reason, LKML has invented yet
another homebrewed crypto primitive. This story really gets more
horrifying every day. No bueno.

So yea, let's revert and re-commit (repeal and replace? just
kidding...). Out with SHA-1, in with Blake2 or SHA2.

Jason

Re: BPF hash algo (Re: [kernel-hardening] Re: [PATCH v7 3/6] random: use SipHash in place of MD5)

[Jason A. Donenfeld]

On Thu, Dec 22, 2016 at 5:59 PM, Hannes Frederic Sowa
 wrote:
> We don't prevent ebpf programs being loaded based on the digest but
> just to uniquely identify loaded programs from user space and match up
> with their source.

Okay, so in that case, a weak hashing function like SHA1 could result
in a real vulnerability. Therefore, this SHA1 stuff needs to be
reverted immediately, pending a different implementation. If this has
ever shipped in a kernel version, it could even deserve a CVE. No
SHA1!

> The hashing is not a proper sha1 neither, unfortunately. I think that
> is why it will have a custom implementation in iproute2?

Jeepers creepers. So for some ungodly reason, LKML has invented yet
another homebrewed crypto primitive. This story really gets more
horrifying every day. No bueno.

So yea, let's revert and re-commit (repeal and replace? just
kidding...). Out with SHA-1, in with Blake2 or SHA2.

Jason

Re: [2/3] serial: 8250: Add new port type for TI DA8xx/OMAPL13x/AM17xx/AM18xx

[David Lechner]

On 12/22/2016 09:21 AM, Franklin S Cooper Jr wrote:



On 12/20/2016 02:23 PM, David Lechner wrote:

This adds a new UART port type for TI DA8xx/OMAPL13x/AM17xx/AM18xx. These
SoCs have standard 8250 registers plus some extra non-standard registers.

The UART will not function unless the non-standard Power and Emulation
Management Register (PWREMU_MGMT) is configured correctly. This is
currently handled in arch/arm/mach-davinci/serial.c for non-device-tree
boards. Making this part of the UART driver will allow UART to work on
device-tree boards as well and the mach code can eventually be removed.

Signed-off-by: David Lechner 
---
 drivers/tty/serial/8250/8250_of.c   |  1 +
 drivers/tty/serial/8250/8250_port.c | 22 ++
 include/uapi/linux/serial_core.h|  3 ++-
 include/uapi/linux/serial_reg.h |  8 
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_of.c 
b/drivers/tty/serial/8250/8250_of.c
index d25ab1c..5281252 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -332,6 +332,7 @@ static const struct of_device_id of_platform_serial_table[] 
= {
.data = (void *)PORT_ALTR_16550_F128, },
{ .compatible = "mrvl,mmp-uart",
.data = (void *)PORT_XSCALE, },
+   { .compatible = "ti,da830-uart", .data = (void *)PORT_DA830, },
{ /* end of list */ },
 };
 MODULE_DEVICE_TABLE(of, of_platform_serial_table);
diff --git a/drivers/tty/serial/8250/8250_port.c 
b/drivers/tty/serial/8250/8250_port.c
index fe4399b..ea854054 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -273,6 +273,15 @@ static const struct serial8250_config uart_config[] = {
.rxtrig_bytes   = {1, 4, 8, 14},
.flags  = UART_CAP_FIFO,
},
+   [PORT_DA830] = {
+   .name   = "TI DA8xx/OMAPL13x/AM17xx/AM18xx",
+   .fifo_size  = 16,
+   .tx_loadsz  = 16,
+   .fcr= UART_FCR_DMA_SELECT | UART_FCR_ENABLE_FIFO |
+ UART_FCR_R_TRIG_10,
+   .rxtrig_bytes   = {1, 4, 8, 14},
+   .flags  = UART_CAP_FIFO | UART_CAP_AFE,
+   },
 };



Any reason why the fcr and flags fields are changed when compared
against PORT_16550A?


The AM1808 TRM says to "always enable" the DMA bit. I figured setting it 
now could save someone trouble later if they wanted to add DMA support. 
It does not matter if it is set even if you are not using DMA.


Since we are using the special reset register that resets the state 
machine, setting UART_FCR_CLEAR_RCVR and UART_FCR_CLEAR_XMIT seems 
redundant.


And in my testing with an AM1808, UART_CAP_AFE is not automatically 
detected even though the chip has this capability, so it needs to be 
manually specified.

Re: [2/3] serial: 8250: Add new port type for TI DA8xx/OMAPL13x/AM17xx/AM18xx

[David Lechner]

On 12/22/2016 09:21 AM, Franklin S Cooper Jr wrote:



On 12/20/2016 02:23 PM, David Lechner wrote:

This adds a new UART port type for TI DA8xx/OMAPL13x/AM17xx/AM18xx. These
SoCs have standard 8250 registers plus some extra non-standard registers.

The UART will not function unless the non-standard Power and Emulation
Management Register (PWREMU_MGMT) is configured correctly. This is
currently handled in arch/arm/mach-davinci/serial.c for non-device-tree
boards. Making this part of the UART driver will allow UART to work on
device-tree boards as well and the mach code can eventually be removed.

Signed-off-by: David Lechner 
---
 drivers/tty/serial/8250/8250_of.c   |  1 +
 drivers/tty/serial/8250/8250_port.c | 22 ++
 include/uapi/linux/serial_core.h|  3 ++-
 include/uapi/linux/serial_reg.h |  8 
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_of.c 
b/drivers/tty/serial/8250/8250_of.c
index d25ab1c..5281252 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -332,6 +332,7 @@ static const struct of_device_id of_platform_serial_table[] 
= {
.data = (void *)PORT_ALTR_16550_F128, },
{ .compatible = "mrvl,mmp-uart",
.data = (void *)PORT_XSCALE, },
+   { .compatible = "ti,da830-uart", .data = (void *)PORT_DA830, },
{ /* end of list */ },
 };
 MODULE_DEVICE_TABLE(of, of_platform_serial_table);
diff --git a/drivers/tty/serial/8250/8250_port.c 
b/drivers/tty/serial/8250/8250_port.c
index fe4399b..ea854054 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -273,6 +273,15 @@ static const struct serial8250_config uart_config[] = {
.rxtrig_bytes   = {1, 4, 8, 14},
.flags  = UART_CAP_FIFO,
},
+   [PORT_DA830] = {
+   .name   = "TI DA8xx/OMAPL13x/AM17xx/AM18xx",
+   .fifo_size  = 16,
+   .tx_loadsz  = 16,
+   .fcr= UART_FCR_DMA_SELECT | UART_FCR_ENABLE_FIFO |
+ UART_FCR_R_TRIG_10,
+   .rxtrig_bytes   = {1, 4, 8, 14},
+   .flags  = UART_CAP_FIFO | UART_CAP_AFE,
+   },
 };



Any reason why the fcr and flags fields are changed when compared
against PORT_16550A?


The AM1808 TRM says to "always enable" the DMA bit. I figured setting it 
now could save someone trouble later if they wanted to add DMA support. 
It does not matter if it is set even if you are not using DMA.


Since we are using the special reset register that resets the state 
machine, setting UART_FCR_CLEAR_RCVR and UART_FCR_CLEAR_XMIT seems 
redundant.


And in my testing with an AM1808, UART_CAP_AFE is not automatically 
detected even though the chip has this capability, so it needs to be 
manually specified.

Re: [PATCH V2 0/2] PM / Domains / OPP: Introduce domain-performance-state binding

[Rob Herring]

On Mon, Dec 12, 2016 at 04:26:17PM +0530, Viresh Kumar wrote:
> Hello,
> 
> Some platforms have the capability to configure the performance state of
> their Power Domains. The performance levels are represented by positive
> integer values, a lower value represents lower performance state.
> 
> We had some discussions about it in the past on the PM list [1], which is
> followed by discussions during the LPC. The outcome of all that was that we
> should extend Power Domain framework to support active state power management
> as well.
> 
> The power-domains until now were only concentrating on the idle state
> management of the device and this needs to change in order to reuse the
> infrastructure of power domains for active state management.

>From a h/w perspective, are idle states really different from 
performance states? 

> 
> To get a complete picture of the proposed plan, following is what we
> need to do:
> - Create DT bindings to get domain performance state information for the
>   platforms.

I would do this last so you can evolve things if you're not certain 
about what the bindings should look like. You can always start with 
things in the kernel and add to DT later.

While in theory we should be able to just "describe the h/w" in DT and 
develop the Linux side independently, this feels too much like the 
bindings are just evolving with Linux needs.

Rob

Re: [kernel-hardening] Re: [PATCH v7 3/6] random: use SipHash in place of MD5

[Jason A. Donenfeld]

On Thu, Dec 22, 2016 at 7:08 PM, Hannes Frederic Sowa
 wrote:
> I wasn't concerned about performance but more about DoS resilience. I
> wonder how safe half md4 actually is in terms of allowing users to
> generate long hash chains in the filesystem (in terms of length
> extension attacks against half_md4).

AFAIK, this is a real vulnerability that needs to be addressed.

Judging by Ted's inquiry about my siphash testing suite, I assume he's
probably tinkering around with it as we speak. :)


Meanwhile I've separated things into several trees:

1. chacha20 rng, already submitted:
https://git.zx2c4.com/linux-dev/log/?h=random-next

2. md5 cleanup, not yet submitted:
https://git.zx2c4.com/linux-dev/log/?h=md5-cleanup

3. md4 cleanup, already submitted:
https://git.zx2c4.com/linux-dev/log/?h=ext4-next-md4-cleanup

4. siphash and networking, not yet submitted as a x/4 series:
https://git.zx2c4.com/linux-dev/log/?h=net-next-siphash

I'll submit (4) in a couple of days, waiting for any comments on the
existing patch-set.

Jason

Re: [PATCH V2 0/2] PM / Domains / OPP: Introduce domain-performance-state binding

[Rob Herring]

On Mon, Dec 12, 2016 at 04:26:17PM +0530, Viresh Kumar wrote:
> Hello,
> 
> Some platforms have the capability to configure the performance state of
> their Power Domains. The performance levels are represented by positive
> integer values, a lower value represents lower performance state.
> 
> We had some discussions about it in the past on the PM list [1], which is
> followed by discussions during the LPC. The outcome of all that was that we
> should extend Power Domain framework to support active state power management
> as well.
> 
> The power-domains until now were only concentrating on the idle state
> management of the device and this needs to change in order to reuse the
> infrastructure of power domains for active state management.

>From a h/w perspective, are idle states really different from 
performance states? 

> 
> To get a complete picture of the proposed plan, following is what we
> need to do:
> - Create DT bindings to get domain performance state information for the
>   platforms.

I would do this last so you can evolve things if you're not certain 
about what the bindings should look like. You can always start with 
things in the kernel and add to DT later.

While in theory we should be able to just "describe the h/w" in DT and 
develop the Linux side independently, this feels too much like the 
bindings are just evolving with Linux needs.

Rob

Re: [kernel-hardening] Re: [PATCH v7 3/6] random: use SipHash in place of MD5

[Jason A. Donenfeld]

On Thu, Dec 22, 2016 at 7:08 PM, Hannes Frederic Sowa
 wrote:
> I wasn't concerned about performance but more about DoS resilience. I
> wonder how safe half md4 actually is in terms of allowing users to
> generate long hash chains in the filesystem (in terms of length
> extension attacks against half_md4).

AFAIK, this is a real vulnerability that needs to be addressed.

Judging by Ted's inquiry about my siphash testing suite, I assume he's
probably tinkering around with it as we speak. :)


Meanwhile I've separated things into several trees:

1. chacha20 rng, already submitted:
https://git.zx2c4.com/linux-dev/log/?h=random-next

2. md5 cleanup, not yet submitted:
https://git.zx2c4.com/linux-dev/log/?h=md5-cleanup

3. md4 cleanup, already submitted:
https://git.zx2c4.com/linux-dev/log/?h=ext4-next-md4-cleanup

4. siphash and networking, not yet submitted as a x/4 series:
https://git.zx2c4.com/linux-dev/log/?h=net-next-siphash

I'll submit (4) in a couple of days, waiting for any comments on the
existing patch-set.

Jason

Re: [kernel-hardening] Re: [PATCH v7 3/6] random: use SipHash in place of MD5

[Hannes Frederic Sowa]

On 22.12.2016 16:54, Theodore Ts'o wrote:
> On Thu, Dec 22, 2016 at 02:10:33PM +0100, Jason A. Donenfeld wrote:
>> On Thu, Dec 22, 2016 at 1:47 PM, Hannes Frederic Sowa
>>  wrote:
>>> following up on what appears to be a random subject: ;)
>>>
>>> IIRC, ext4 code by default still uses half_md4 for hashing of filenames
>>> in the htree. siphash seems to fit this use case pretty good.
>>
>> I saw this too. I'll try to address it in v8 of this series.
> 
> This is a separate issue, and this series is getting a bit too
> complex.  So I'd suggest pushing this off to a separate change.
> 
> Changing the htree hash algorithm is an on-disk format change, and so
> we couldn't roll it out until e2fsprogs gets updated and rolled out
> pretty broadley.  In fact George sent me patches to add siphash as a
> hash algorithm for htree a while back (for both the kernel and
> e2fsprogs), but I never got around to testing and applying them,
> mainly because while it's technically faster, I had other higher
> priority issues to work on --- and see previous comments regarding
> pixel peeping.  Improving the hash algorithm by tens or even hundreds
> of nanoseconds isn't really going to matter since we only do a htree
> lookup on a file creation or cold cache lookup, and the SSD or HDD I/O
> times will dominate.  And from the power perspective, saving
> microwatts of CPU power isn't going to matter if you're going to be
> spinning up the storage device

I wasn't concerned about performance but more about DoS resilience. I
wonder how safe half md4 actually is in terms of allowing users to
generate long hash chains in the filesystem (in terms of length
extension attacks against half_md4).

In ext4, is it actually possible that a "disrupter" learns about the
hashing secret in the way how the inodes are returned during getdents?

Thanks,
Hannes

Re: [kernel-hardening] Re: [PATCH v7 3/6] random: use SipHash in place of MD5

[Hannes Frederic Sowa]

On 22.12.2016 16:54, Theodore Ts'o wrote:
> On Thu, Dec 22, 2016 at 02:10:33PM +0100, Jason A. Donenfeld wrote:
>> On Thu, Dec 22, 2016 at 1:47 PM, Hannes Frederic Sowa
>>  wrote:
>>> following up on what appears to be a random subject: ;)
>>>
>>> IIRC, ext4 code by default still uses half_md4 for hashing of filenames
>>> in the htree. siphash seems to fit this use case pretty good.
>>
>> I saw this too. I'll try to address it in v8 of this series.
> 
> This is a separate issue, and this series is getting a bit too
> complex.  So I'd suggest pushing this off to a separate change.
> 
> Changing the htree hash algorithm is an on-disk format change, and so
> we couldn't roll it out until e2fsprogs gets updated and rolled out
> pretty broadley.  In fact George sent me patches to add siphash as a
> hash algorithm for htree a while back (for both the kernel and
> e2fsprogs), but I never got around to testing and applying them,
> mainly because while it's technically faster, I had other higher
> priority issues to work on --- and see previous comments regarding
> pixel peeping.  Improving the hash algorithm by tens or even hundreds
> of nanoseconds isn't really going to matter since we only do a htree
> lookup on a file creation or cold cache lookup, and the SSD or HDD I/O
> times will dominate.  And from the power perspective, saving
> microwatts of CPU power isn't going to matter if you're going to be
> spinning up the storage device

I wasn't concerned about performance but more about DoS resilience. I
wonder how safe half md4 actually is in terms of allowing users to
generate long hash chains in the filesystem (in terms of length
extension attacks against half_md4).

In ext4, is it actually possible that a "disrupter" learns about the
hashing secret in the way how the inodes are returned during getdents?

Thanks,
Hannes

[PATCH 2/2] random: convert get_random_int/long into get_random_u32/u64

[Jason A. Donenfeld]

Many times, when a user wants a random number, he wants a random number
of a guaranteed size. So, thinking of get_random_int and get_random_long
in terms of get_random_u32 and get_random_u64 makes it much easier to
achieve this. It also makes the code simpler.

On 32-bit platforms, get_random_int and get_random_long are both aliased
to get_random_u32. On 64-bit platforms, int->u32 and long->u64.

Signed-off-by: Jason A. Donenfeld 
Cc: Theodore Ts'o 
---
I personally would find this extremely useful in my own code, where I
currently wind up doing something ugly like:

(u64)get_random_int() << 32 | get_random_int()

But if you have some aversion, don't hesitate to just take 1/2 without
this 2/2. But hopefully you'll like this 2/2.

 drivers/char/random.c  | 55 +-
 include/linux/random.h | 17 ++--
 2 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 08d1dd58c0d2..ee737ef41ce9 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -2044,8 +2044,8 @@ struct ctl_table random_table[] = {
 
 struct batched_entropy {
union {
-   unsigned long entropy_long[CHACHA20_BLOCK_SIZE / 
sizeof(unsigned long)];
-   unsigned int entropy_int[CHACHA20_BLOCK_SIZE / sizeof(unsigned 
int)];
+   u64 entropy_u64[CHACHA20_BLOCK_SIZE / sizeof(u64)];
+   u32 entropy_u32[CHACHA20_BLOCK_SIZE / sizeof(u32)];
};
unsigned int position;
 };
@@ -2055,52 +2055,51 @@ struct batched_entropy {
  * number is either as good as RDRAND or as good as /dev/urandom, with the
  * goal of being quite fast and not depleting entropy.
  */
-static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_long);
-unsigned long get_random_long(void)
+static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u64);
+u64 get_random_u64(void)
 {
-   unsigned long ret;
+   u64 ret;
struct batched_entropy *batch;
 
-   if (arch_get_random_long())
+#if BITS_PER_LONG == 64
+   if (arch_get_random_long((unsigned long *)))
return ret;
+#else
+   if (arch_get_random_long((unsigned long *)) &&
+   arch_get_random_long((unsigned long *) + 1))
+   return ret;
+#endif
 
-   batch = _cpu_var(batched_entropy_long);
-   if (batch->position % ARRAY_SIZE(batch->entropy_long) == 0) {
-   extract_crng((u8 *)batch->entropy_long);
+   batch = _cpu_var(batched_entropy_u64);
+   if (batch->position % ARRAY_SIZE(batch->entropy_u64) == 0) {
+   extract_crng((u8 *)batch->entropy_u64);
batch->position = 0;
}
-   ret = batch->entropy_long[batch->position++];
-   put_cpu_var(batched_entropy_long);
+   ret = batch->entropy_u64[batch->position++];
+   put_cpu_var(batched_entropy_u64);
return ret;
 }
-EXPORT_SYMBOL(get_random_long);
+EXPORT_SYMBOL(get_random_u64);
 
-#if BITS_PER_LONG == 32
-unsigned int get_random_int(void)
-{
-   return get_random_long();
-}
-#else
-static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_int);
-unsigned int get_random_int(void)
+static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u32);
+u32 get_random_u32(void)
 {
-   unsigned int ret;
+   u32 ret;
struct batched_entropy *batch;
 
if (arch_get_random_int())
return ret;
 
-   batch = _cpu_var(batched_entropy_int);
-   if (batch->position % ARRAY_SIZE(batch->entropy_int) == 0) {
-   extract_crng((u8 *)batch->entropy_int);
+   batch = _cpu_var(batched_entropy_u32);
+   if (batch->position % ARRAY_SIZE(batch->entropy_u32) == 0) {
+   extract_crng((u8 *)batch->entropy_u32);
batch->position = 0;
}
-   ret = batch->entropy_int[batch->position++];
-   put_cpu_var(batched_entropy_int);
+   ret = batch->entropy_u32[batch->position++];
+   put_cpu_var(batched_entropy_u32);
return ret;
 }
-#endif
-EXPORT_SYMBOL(get_random_int);
+EXPORT_SYMBOL(get_random_u32);
 
 /**
  * randomize_page - Generate a random, page aligned address
diff --git a/include/linux/random.h b/include/linux/random.h
index 16ab429735a7..ed5c3838780d 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -42,8 +42,21 @@ extern void get_random_bytes_arch(void *buf, int nbytes);
 extern const struct file_operations random_fops, urandom_fops;
 #endif
 
-unsigned int get_random_int(void);
-unsigned long get_random_long(void);
+u32 get_random_u32(void);
+u64 get_random_u64(void);
+static inline unsigned int get_random_int(void)
+{
+   return get_random_u32();
+}
+static inline unsigned long get_random_long(void)
+{
+#if BITS_PER_LONG == 64
+   return get_random_u64();
+#else
+   return get_random_u32();
+#endif
+}
+
 unsigned long randomize_page(unsigned long start, unsigned long range);
 
 u32 

[PATCH 1/2] random: use chacha20 for get_random_int/long

[Jason A. Donenfeld]

Now that our crng uses chacha20, we can rely on its speedy
characteristics for replacing MD5, while simultaneously achieving a
higher security guarantee. Before the idea was to use these functions if
you wanted random integers that aren't stupidly insecure but aren't
necessarily secure either, a vague gray zone, that hopefully was "good
enough" for its users. With chacha20, we can strengthen this claim,
since either we're using an rdrand-like instruction, or we're using the
same crng as /dev/urandom. And it's faster than what was before.

We could have chosen to replace this with a SipHash-derived function,
which might be slightly faster, but at the cost of having yet another
RNG construction in the kernel. By moving to chacha20, we have a single
RNG to analyze and verify, and we also already get good performance
improvements on all platforms.

Implementation-wise, rather than use a generic buffer for both
get_random_int/long and memcpy based on the size needs, we use a
specific buffer for 32-bit reads and for 64-bit reads. This way, we're
guaranteed to always have aligned accesses on all platforms. While
slightly more verbose in C, the assembly this generates is a lot
simpler than otherwise.

Finally, on 32-bit platforms where longs and ints are the same size,
we simply alias get_random_int to get_random_long.

Signed-off-by: Jason A. Donenfeld 
Suggested-by: Theodore Ts'o 
Cc: Theodore Ts'o 
Cc: Hannes Frederic Sowa 
Cc: Andy Lutomirski 
---
 drivers/char/random.c  | 84 ++
 include/linux/random.h |  1 -
 init/main.c|  1 -
 3 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index d6876d506220..08d1dd58c0d2 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -2042,63 +2042,65 @@ struct ctl_table random_table[] = {
 };
 #endif /* CONFIG_SYSCTL */
 
-static u32 random_int_secret[MD5_MESSAGE_BYTES / 4] cacheline_aligned;
-
-int random_int_secret_init(void)
-{
-   get_random_bytes(random_int_secret, sizeof(random_int_secret));
-   return 0;
-}
-
-static DEFINE_PER_CPU(__u32 [MD5_DIGEST_WORDS], get_random_int_hash)
-   __aligned(sizeof(unsigned long));
+struct batched_entropy {
+   union {
+   unsigned long entropy_long[CHACHA20_BLOCK_SIZE / 
sizeof(unsigned long)];
+   unsigned int entropy_int[CHACHA20_BLOCK_SIZE / sizeof(unsigned 
int)];
+   };
+   unsigned int position;
+};
 
 /*
- * Get a random word for internal kernel use only. Similar to urandom but
- * with the goal of minimal entropy pool depletion. As a result, the random
- * value is not cryptographically secure but for several uses the cost of
- * depleting entropy is too high
+ * Get a random word for internal kernel use only. The quality of the random
+ * number is either as good as RDRAND or as good as /dev/urandom, with the
+ * goal of being quite fast and not depleting entropy.
  */
-unsigned int get_random_int(void)
+static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_long);
+unsigned long get_random_long(void)
 {
-   __u32 *hash;
-   unsigned int ret;
+   unsigned long ret;
+   struct batched_entropy *batch;
 
-   if (arch_get_random_int())
+   if (arch_get_random_long())
return ret;
 
-   hash = get_cpu_var(get_random_int_hash);
-
-   hash[0] += current->pid + jiffies + random_get_entropy();
-   md5_transform(hash, random_int_secret);
-   ret = hash[0];
-   put_cpu_var(get_random_int_hash);
-
+   batch = _cpu_var(batched_entropy_long);
+   if (batch->position % ARRAY_SIZE(batch->entropy_long) == 0) {
+   extract_crng((u8 *)batch->entropy_long);
+   batch->position = 0;
+   }
+   ret = batch->entropy_long[batch->position++];
+   put_cpu_var(batched_entropy_long);
return ret;
 }
-EXPORT_SYMBOL(get_random_int);
+EXPORT_SYMBOL(get_random_long);
 
-/*
- * Same as get_random_int(), but returns unsigned long.
- */
-unsigned long get_random_long(void)
+#if BITS_PER_LONG == 32
+unsigned int get_random_int(void)
 {
-   __u32 *hash;
-   unsigned long ret;
+   return get_random_long();
+}
+#else
+static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_int);
+unsigned int get_random_int(void)
+{
+   unsigned int ret;
+   struct batched_entropy *batch;
 
-   if (arch_get_random_long())
+   if (arch_get_random_int())
return ret;
 
-   hash = get_cpu_var(get_random_int_hash);
-
-   hash[0] += current->pid + jiffies + random_get_entropy();
-   md5_transform(hash, random_int_secret);
-   ret = *(unsigned long *)hash;
-   put_cpu_var(get_random_int_hash);
-
+   batch = _cpu_var(batched_entropy_int);
+   if (batch->position % ARRAY_SIZE(batch->entropy_int) == 0) {
+   

[PATCH 2/2] random: convert get_random_int/long into get_random_u32/u64

[Jason A. Donenfeld]

Many times, when a user wants a random number, he wants a random number
of a guaranteed size. So, thinking of get_random_int and get_random_long
in terms of get_random_u32 and get_random_u64 makes it much easier to
achieve this. It also makes the code simpler.

On 32-bit platforms, get_random_int and get_random_long are both aliased
to get_random_u32. On 64-bit platforms, int->u32 and long->u64.

Signed-off-by: Jason A. Donenfeld 
Cc: Theodore Ts'o 
---
I personally would find this extremely useful in my own code, where I
currently wind up doing something ugly like:

(u64)get_random_int() << 32 | get_random_int()

But if you have some aversion, don't hesitate to just take 1/2 without
this 2/2. But hopefully you'll like this 2/2.

 drivers/char/random.c  | 55 +-
 include/linux/random.h | 17 ++--
 2 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 08d1dd58c0d2..ee737ef41ce9 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -2044,8 +2044,8 @@ struct ctl_table random_table[] = {
 
 struct batched_entropy {
union {
-   unsigned long entropy_long[CHACHA20_BLOCK_SIZE / 
sizeof(unsigned long)];
-   unsigned int entropy_int[CHACHA20_BLOCK_SIZE / sizeof(unsigned 
int)];
+   u64 entropy_u64[CHACHA20_BLOCK_SIZE / sizeof(u64)];
+   u32 entropy_u32[CHACHA20_BLOCK_SIZE / sizeof(u32)];
};
unsigned int position;
 };
@@ -2055,52 +2055,51 @@ struct batched_entropy {
  * number is either as good as RDRAND or as good as /dev/urandom, with the
  * goal of being quite fast and not depleting entropy.
  */
-static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_long);
-unsigned long get_random_long(void)
+static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u64);
+u64 get_random_u64(void)
 {
-   unsigned long ret;
+   u64 ret;
struct batched_entropy *batch;
 
-   if (arch_get_random_long())
+#if BITS_PER_LONG == 64
+   if (arch_get_random_long((unsigned long *)))
return ret;
+#else
+   if (arch_get_random_long((unsigned long *)) &&
+   arch_get_random_long((unsigned long *) + 1))
+   return ret;
+#endif
 
-   batch = _cpu_var(batched_entropy_long);
-   if (batch->position % ARRAY_SIZE(batch->entropy_long) == 0) {
-   extract_crng((u8 *)batch->entropy_long);
+   batch = _cpu_var(batched_entropy_u64);
+   if (batch->position % ARRAY_SIZE(batch->entropy_u64) == 0) {
+   extract_crng((u8 *)batch->entropy_u64);
batch->position = 0;
}
-   ret = batch->entropy_long[batch->position++];
-   put_cpu_var(batched_entropy_long);
+   ret = batch->entropy_u64[batch->position++];
+   put_cpu_var(batched_entropy_u64);
return ret;
 }
-EXPORT_SYMBOL(get_random_long);
+EXPORT_SYMBOL(get_random_u64);
 
-#if BITS_PER_LONG == 32
-unsigned int get_random_int(void)
-{
-   return get_random_long();
-}
-#else
-static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_int);
-unsigned int get_random_int(void)
+static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u32);
+u32 get_random_u32(void)
 {
-   unsigned int ret;
+   u32 ret;
struct batched_entropy *batch;
 
if (arch_get_random_int())
return ret;
 
-   batch = _cpu_var(batched_entropy_int);
-   if (batch->position % ARRAY_SIZE(batch->entropy_int) == 0) {
-   extract_crng((u8 *)batch->entropy_int);
+   batch = _cpu_var(batched_entropy_u32);
+   if (batch->position % ARRAY_SIZE(batch->entropy_u32) == 0) {
+   extract_crng((u8 *)batch->entropy_u32);
batch->position = 0;
}
-   ret = batch->entropy_int[batch->position++];
-   put_cpu_var(batched_entropy_int);
+   ret = batch->entropy_u32[batch->position++];
+   put_cpu_var(batched_entropy_u32);
return ret;
 }
-#endif
-EXPORT_SYMBOL(get_random_int);
+EXPORT_SYMBOL(get_random_u32);
 
 /**
  * randomize_page - Generate a random, page aligned address
diff --git a/include/linux/random.h b/include/linux/random.h
index 16ab429735a7..ed5c3838780d 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -42,8 +42,21 @@ extern void get_random_bytes_arch(void *buf, int nbytes);
 extern const struct file_operations random_fops, urandom_fops;
 #endif
 
-unsigned int get_random_int(void);
-unsigned long get_random_long(void);
+u32 get_random_u32(void);
+u64 get_random_u64(void);
+static inline unsigned int get_random_int(void)
+{
+   return get_random_u32();
+}
+static inline unsigned long get_random_long(void)
+{
+#if BITS_PER_LONG == 64
+   return get_random_u64();
+#else
+   return get_random_u32();
+#endif
+}
+
 unsigned long randomize_page(unsigned long start, unsigned long range);
 
 u32 prandom_u32(void);
-- 
2.11.0

[PATCH 1/2] random: use chacha20 for get_random_int/long

[Jason A. Donenfeld]

Now that our crng uses chacha20, we can rely on its speedy
characteristics for replacing MD5, while simultaneously achieving a
higher security guarantee. Before the idea was to use these functions if
you wanted random integers that aren't stupidly insecure but aren't
necessarily secure either, a vague gray zone, that hopefully was "good
enough" for its users. With chacha20, we can strengthen this claim,
since either we're using an rdrand-like instruction, or we're using the
same crng as /dev/urandom. And it's faster than what was before.

We could have chosen to replace this with a SipHash-derived function,
which might be slightly faster, but at the cost of having yet another
RNG construction in the kernel. By moving to chacha20, we have a single
RNG to analyze and verify, and we also already get good performance
improvements on all platforms.

Implementation-wise, rather than use a generic buffer for both
get_random_int/long and memcpy based on the size needs, we use a
specific buffer for 32-bit reads and for 64-bit reads. This way, we're
guaranteed to always have aligned accesses on all platforms. While
slightly more verbose in C, the assembly this generates is a lot
simpler than otherwise.

Finally, on 32-bit platforms where longs and ints are the same size,
we simply alias get_random_int to get_random_long.

Signed-off-by: Jason A. Donenfeld 
Suggested-by: Theodore Ts'o 
Cc: Theodore Ts'o 
Cc: Hannes Frederic Sowa 
Cc: Andy Lutomirski 
---
 drivers/char/random.c  | 84 ++
 include/linux/random.h |  1 -
 init/main.c|  1 -
 3 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index d6876d506220..08d1dd58c0d2 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -2042,63 +2042,65 @@ struct ctl_table random_table[] = {
 };
 #endif /* CONFIG_SYSCTL */
 
-static u32 random_int_secret[MD5_MESSAGE_BYTES / 4] cacheline_aligned;
-
-int random_int_secret_init(void)
-{
-   get_random_bytes(random_int_secret, sizeof(random_int_secret));
-   return 0;
-}
-
-static DEFINE_PER_CPU(__u32 [MD5_DIGEST_WORDS], get_random_int_hash)
-   __aligned(sizeof(unsigned long));
+struct batched_entropy {
+   union {
+   unsigned long entropy_long[CHACHA20_BLOCK_SIZE / 
sizeof(unsigned long)];
+   unsigned int entropy_int[CHACHA20_BLOCK_SIZE / sizeof(unsigned 
int)];
+   };
+   unsigned int position;
+};
 
 /*
- * Get a random word for internal kernel use only. Similar to urandom but
- * with the goal of minimal entropy pool depletion. As a result, the random
- * value is not cryptographically secure but for several uses the cost of
- * depleting entropy is too high
+ * Get a random word for internal kernel use only. The quality of the random
+ * number is either as good as RDRAND or as good as /dev/urandom, with the
+ * goal of being quite fast and not depleting entropy.
  */
-unsigned int get_random_int(void)
+static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_long);
+unsigned long get_random_long(void)
 {
-   __u32 *hash;
-   unsigned int ret;
+   unsigned long ret;
+   struct batched_entropy *batch;
 
-   if (arch_get_random_int())
+   if (arch_get_random_long())
return ret;
 
-   hash = get_cpu_var(get_random_int_hash);
-
-   hash[0] += current->pid + jiffies + random_get_entropy();
-   md5_transform(hash, random_int_secret);
-   ret = hash[0];
-   put_cpu_var(get_random_int_hash);
-
+   batch = _cpu_var(batched_entropy_long);
+   if (batch->position % ARRAY_SIZE(batch->entropy_long) == 0) {
+   extract_crng((u8 *)batch->entropy_long);
+   batch->position = 0;
+   }
+   ret = batch->entropy_long[batch->position++];
+   put_cpu_var(batched_entropy_long);
return ret;
 }
-EXPORT_SYMBOL(get_random_int);
+EXPORT_SYMBOL(get_random_long);
 
-/*
- * Same as get_random_int(), but returns unsigned long.
- */
-unsigned long get_random_long(void)
+#if BITS_PER_LONG == 32
+unsigned int get_random_int(void)
 {
-   __u32 *hash;
-   unsigned long ret;
+   return get_random_long();
+}
+#else
+static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_int);
+unsigned int get_random_int(void)
+{
+   unsigned int ret;
+   struct batched_entropy *batch;
 
-   if (arch_get_random_long())
+   if (arch_get_random_int())
return ret;
 
-   hash = get_cpu_var(get_random_int_hash);
-
-   hash[0] += current->pid + jiffies + random_get_entropy();
-   md5_transform(hash, random_int_secret);
-   ret = *(unsigned long *)hash;
-   put_cpu_var(get_random_int_hash);
-
+   batch = _cpu_var(batched_entropy_int);
+   if (batch->position % ARRAY_SIZE(batch->entropy_int) == 0) {
+   extract_crng((u8 *)batch->entropy_int);
+   batch->position = 0;
+   }

Re: [PATCH v4 0/2] power: supply: add sbs-charger driver

[Sebastian Reichel]

Hi,

On Tue, Dec 20, 2016 at 04:31:12PM +0100, Nicolas Saenz Julienne wrote:
> This series adds support for all SBS compatible battery chargers, as defined
> here: http://sbs-forum.org/specs/sbc110.pdf.
>
> [...]
>
> Nicolas Saenz Julienne (2):
>   power: supply: add sbs-charger driver
>   dt-bindings: power: add bindings for sbs-charger
> 
>  .../bindings/power/supply/sbs_sbs-charger.txt  |  23 ++
>  drivers/power/supply/Kconfig   |   6 +
>  drivers/power/supply/Makefile  |   1 +
>  drivers/power/supply/sbs-charger.c | 274 
> +
>  4 files changed, 304 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/power/supply/sbs_sbs-charger.txt
>  create mode 100644 drivers/power/supply/sbs-charger.c

Thanks for your patchset. We are currently in the merge
window and your patchset will appear in linux-next once
4.10-rc1 has been tagged by Linus Torvalds.

Until then I queued it into this branch:

https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next-next

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v4 0/2] power: supply: add sbs-charger driver

[Sebastian Reichel]

Hi,

On Tue, Dec 20, 2016 at 04:31:12PM +0100, Nicolas Saenz Julienne wrote:
> This series adds support for all SBS compatible battery chargers, as defined
> here: http://sbs-forum.org/specs/sbc110.pdf.
>
> [...]
>
> Nicolas Saenz Julienne (2):
>   power: supply: add sbs-charger driver
>   dt-bindings: power: add bindings for sbs-charger
> 
>  .../bindings/power/supply/sbs_sbs-charger.txt  |  23 ++
>  drivers/power/supply/Kconfig   |   6 +
>  drivers/power/supply/Makefile  |   1 +
>  drivers/power/supply/sbs-charger.c | 274 
> +
>  4 files changed, 304 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/power/supply/sbs_sbs-charger.txt
>  create mode 100644 drivers/power/supply/sbs-charger.c

Thanks for your patchset. We are currently in the merge
window and your patchset will appear in linux-next once
4.10-rc1 has been tagged by Linus Torvalds.

Until then I queued it into this branch:

https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next-next

-- Sebastian


signature.asc
Description: PGP signature

Re: [patch 00/10] cpu/hotplug: Final cleanup

[Sam Ravnborg]

Hi Thomas.

On Wed, Dec 21, 2016 at 08:19:47PM +0100, Thomas Gleixner wrote:
> The following series completes the cpuhotplug rework stage ONE.

Just curious - what is the stage TWO (and onwards) plan?
lwn had some coverage of this work but I assume the
article is somewhat outdated.

Sam

Re: [patch 00/10] cpu/hotplug: Final cleanup

[Sam Ravnborg]

Hi Thomas.

On Wed, Dec 21, 2016 at 08:19:47PM +0100, Thomas Gleixner wrote:
> The following series completes the cpuhotplug rework stage ONE.

Just curious - what is the stage TWO (and onwards) plan?
lwn had some coverage of this work but I assume the
article is somewhat outdated.

Sam

Re: [PATCH net-next] ixgbevf: fix 'Etherleak' in ixgbevf

[Alexander Duyck]

Yes that is much more helpful.  So looking at it things are being
padded but the last 4 bytes always have this extra data in them.  I've
been trying to recreate the issue on an 82599 with an SR-IOV VF and I
haven't been having much luck reproducing the problem.

In your test environment is the 82599 connected directly to the
Windows machine or are there any
switches/routers/gateways/tunnels/vlans in between?  I've tried
several iterations but with the 82599 connected directly to another
NIC I have I am not able to get it to produce the garbage padding you
are seeing. It makes me wonder if there might be something that is
manipulating the packets in between the two systems.  For example if
there was a VLAN being associated with the VF that is later stripped
and then the packet handed raw to the test system it might explain
what is introducing the extra padding and reason for pulling in the
CRC, and your patch would mask the issue since it would push the
minimum frame size with a VLAN to 68 instead of 64.

- Alex

On Wed, Dec 21, 2016 at 6:00 PM, Kefeng Wang  wrote:
>
>
> On 2016/12/21 10:20, Alexander Duyck wrote:
>> I find it curious that only the last 4 bytes have data in them.  I'm
>> wondering if the NIC/driver in the Windows/Nessus system is
>> interpreting the 4 byte CRC on the end of the frame as padding instead
>> of stripping it.
>>
>> Is there any chance you could capture the entire frame instead of just
>> the padding?  Maybe you could run something like wireshark without
>> enabling promiscuous mode on the VF and capture the frames it is
>> trying to send and receive.  What I want to verify is what the actual
>> amount of padding is that is needed to get to 60 bytes and where the
>> CRC should start.
>>
>> - Alex
>
> Here is the verbose output, is this useful?
> Or we will try according to your advice, thanks,
>
> D:\Program Files\Tenable\Nessus>nasl.exe -aX -t 192.169.0.151 etherleak.nasl
> --
>  ---[ ICMP ]---
> 0x00:  45 00 00 1D 20 81 00 00 40 01 D7 F3 C0 A9 00 97E... ...@...
> 0x10:  C0 A9 00 82 00 00 87 FD 00 01 00 01 78 00 00 00x...
> 0x20:  00 00 00 00 00 00 00 00 00 00 98 E4 75 DF  u.
> --
>  ---[ ICMP ]---
> 0x00:  45 00 00 1D 20 85 00 00 40 01 D7 EF C0 A9 00 97E... ...@...
> 0x10:  C0 A9 00 82 00 00 87 FD 00 01 00 01 78 00 00 00x...
> 0x20:  00 00 00 00 00 00 00 00 00 00 FB DA F8 13  ..
> ---[ ether1 ]---
> 0x00:  00 00 00 00 00 00 00 00 00 00 00 00 00 98 E4 75...u
> 0x10:  DF .
> ---[ ether2 ]---
> 0x00:  00 00 00 00 00 00 00 00 00 00 00 00 00 FB DA F8
> 0x10:  13 .
>
> Padding observed in one frame :
>
>   0x00:  00 00 00 00 00 00 00 00 00 00 00 00 00 98 E4 75...u
>   0x10:  DF .
>
> Padding observed in another frame :
>
>   0x00:  00 00 00 00 00 00 00 00 00 00 00 00 00 FB DA F8
>   0x10:  13
>
>

Re: [PATCH net-next] ixgbevf: fix 'Etherleak' in ixgbevf

[Alexander Duyck]

Yes that is much more helpful.  So looking at it things are being
padded but the last 4 bytes always have this extra data in them.  I've
been trying to recreate the issue on an 82599 with an SR-IOV VF and I
haven't been having much luck reproducing the problem.

In your test environment is the 82599 connected directly to the
Windows machine or are there any
switches/routers/gateways/tunnels/vlans in between?  I've tried
several iterations but with the 82599 connected directly to another
NIC I have I am not able to get it to produce the garbage padding you
are seeing. It makes me wonder if there might be something that is
manipulating the packets in between the two systems.  For example if
there was a VLAN being associated with the VF that is later stripped
and then the packet handed raw to the test system it might explain
what is introducing the extra padding and reason for pulling in the
CRC, and your patch would mask the issue since it would push the
minimum frame size with a VLAN to 68 instead of 64.

- Alex

On Wed, Dec 21, 2016 at 6:00 PM, Kefeng Wang  wrote:
>
>
> On 2016/12/21 10:20, Alexander Duyck wrote:
>> I find it curious that only the last 4 bytes have data in them.  I'm
>> wondering if the NIC/driver in the Windows/Nessus system is
>> interpreting the 4 byte CRC on the end of the frame as padding instead
>> of stripping it.
>>
>> Is there any chance you could capture the entire frame instead of just
>> the padding?  Maybe you could run something like wireshark without
>> enabling promiscuous mode on the VF and capture the frames it is
>> trying to send and receive.  What I want to verify is what the actual
>> amount of padding is that is needed to get to 60 bytes and where the
>> CRC should start.
>>
>> - Alex
>
> Here is the verbose output, is this useful?
> Or we will try according to your advice, thanks,
>
> D:\Program Files\Tenable\Nessus>nasl.exe -aX -t 192.169.0.151 etherleak.nasl
> --
>  ---[ ICMP ]---
> 0x00:  45 00 00 1D 20 81 00 00 40 01 D7 F3 C0 A9 00 97E... ...@...
> 0x10:  C0 A9 00 82 00 00 87 FD 00 01 00 01 78 00 00 00x...
> 0x20:  00 00 00 00 00 00 00 00 00 00 98 E4 75 DF  u.
> --
>  ---[ ICMP ]---
> 0x00:  45 00 00 1D 20 85 00 00 40 01 D7 EF C0 A9 00 97E... ...@...
> 0x10:  C0 A9 00 82 00 00 87 FD 00 01 00 01 78 00 00 00x...
> 0x20:  00 00 00 00 00 00 00 00 00 00 FB DA F8 13  ..
> ---[ ether1 ]---
> 0x00:  00 00 00 00 00 00 00 00 00 00 00 00 00 98 E4 75...u
> 0x10:  DF .
> ---[ ether2 ]---
> 0x00:  00 00 00 00 00 00 00 00 00 00 00 00 00 FB DA F8
> 0x10:  13 .
>
> Padding observed in one frame :
>
>   0x00:  00 00 00 00 00 00 00 00 00 00 00 00 00 98 E4 75...u
>   0x10:  DF .
>
> Padding observed in another frame :
>
>   0x00:  00 00 00 00 00 00 00 00 00 00 00 00 00 FB DA F8
>   0x10:  13
>
>

Re: BPF hash algo (Re: [kernel-hardening] Re: [PATCH v7 3/6] random: use SipHash in place of MD5)

[Hannes Frederic Sowa]

On Thu, 2016-12-22 at 09:25 -0800, Andy Lutomirski wrote:
> On Thu, Dec 22, 2016 at 8:59 AM, Hannes Frederic Sowa
>  wrote:
> > On Thu, 2016-12-22 at 08:07 -0800, Andy Lutomirski wrote:
> > > 
> > > You mean:
> > > 
> > > commit 7bd509e311f408f7a5132fcdde2069af65fa05ae
> > > Author: Daniel Borkmann 
> > > Date:   Sun Dec 4 23:19:41 2016 +0100
> > > 
> > > bpf: add prog_digest and expose it via fdinfo/netlink
> > > 
> > > 
> > > Yes, please!  This actually matters for security -- imagine a
> > > malicious program brute-forcing a collision so that it gets loaded
> > > wrong.  And this is IMO a use case for SHA-256 or SHA-512/256
> > > (preferably the latter).  Speed basically doesn't matter here and
> > > Blake2 is both less stable (didn't they slightly change it recently?)
> > > and much less well studied.
> > 
> > We don't prevent ebpf programs being loaded based on the digest but
> > just to uniquely identify loaded programs from user space and match up
> > with their source.
> 
> The commit log talks about using the hash to see if the program has
> already been compiled and JITted.  If that's done, then a collision
> will directly cause the kernel to malfunction.

Yeah, it still shouldn't crash the kernel but it could cause
malfunctions because assumptions are not met from user space thus it
could act in a strange way:

My personal biggest concern is that users of this API will at some
point in time assume this digist is unique (as a key itself for a
hashtables f.e.), while it is actually not (and not enforced so by the
kernel). If you can get an unpriv ebpf program inserted to the kernel
with the same weak hash, a controller daemon could pick it up and bind
it to another ebpf hook, probably outside of the unpriv realm the user
was in before. Only the sorting matters, which might be unstable and is
not guaranteed by anything in most hash table based data structures.

The API seems flawed to me.

> > > My inclination would have been to seed them with something that isn't
> > > exposed to userspace for the precise reason that it would prevent user
> > > code from making assumptions about what's in the hash.  But if there's
> > > a use case for why user code needs to be able to calculate the hash on
> > > its own, then that's fine.  But perhaps the actual fdinfo string
> > > should be "sha256:abcd1234..." to give some flexibility down the road.

To add to this, I am very much in favor of that. Right now it doesn't
have a name because it is a custom algorithm. ;)

> > > 
> > > Also:
> > > 
> > > +   result = (__force __be32 *)fp->digest;
> > > +   for (i = 0; i < SHA_DIGEST_WORDS; i++)
> > > +   result[i] = cpu_to_be32(fp->digest[i]);
> > > 
> > > Everyone, please, please, please don't open-code crypto primitives.
> > > Is this and the code above it even correct?  It might be but on a very
> > > brief glance it looks wrong to me.  If you're doing this to avoid
> > > depending on crypto, then fix crypto so you can pull in the algorithm
> > > without pulling in the whole crypto core.
> > 
> > The hashing is not a proper sha1 neither, unfortunately. I think that
> > is why it will have a custom implementation in iproute2?
> 
> Putting on crypto hat:
> 
> NAK NAK NAK NAK NAK.  "The Linux kernel invented a new primitive in
> 2016 when people know better and is going to handle it by porting that
> new primitive to userspace" is not a particularly good argument.
> 
> Okay, crypto hack back off.
> 
> > 
> > I wondered if bpf program loading should have used the module loading
> > infrastructure from the beginning...
> 
> That would be way too complicated and would be nasty for the unprivileged 
> cases.

I was more or less just thinking about using the syscalls and user
space representation not the generic infrastructure, as it is anyway
too much concerned with real kernel modules (would probably also solve
your cgroup-ebpf thread, as it can be passed by unique name or name and
hash ;) ). Anyway...

> > > At the very least, there should be a separate function that calculates
> > > the hash of a buffer and that function should explicitly run itself
> > > against test vectors of various lengths to make sure that it's
> > > calculating what it claims to be calculating.  And it doesn't look
> > > like the userspace code has landed, so, if this thing isn't
> > > calculating SHA1 correctly, it's plausible that no one has noticed.
> > 
> > I hope this was known from the beginning, this is not sha1 unfortunately.
> > 
> > But ebpf elf programs also need preprocessing to get rid of some
> > embedded load-depending data, so maybe it was considered to be just
> > enough?
> 
> I suspect it was actually an accident.

Maybe, I don't know.

Bye,
Hannes

Re: BPF hash algo (Re: [kernel-hardening] Re: [PATCH v7 3/6] random: use SipHash in place of MD5)

[Hannes Frederic Sowa]

On Thu, 2016-12-22 at 09:25 -0800, Andy Lutomirski wrote:
> On Thu, Dec 22, 2016 at 8:59 AM, Hannes Frederic Sowa
>  wrote:
> > On Thu, 2016-12-22 at 08:07 -0800, Andy Lutomirski wrote:
> > > 
> > > You mean:
> > > 
> > > commit 7bd509e311f408f7a5132fcdde2069af65fa05ae
> > > Author: Daniel Borkmann 
> > > Date:   Sun Dec 4 23:19:41 2016 +0100
> > > 
> > > bpf: add prog_digest and expose it via fdinfo/netlink
> > > 
> > > 
> > > Yes, please!  This actually matters for security -- imagine a
> > > malicious program brute-forcing a collision so that it gets loaded
> > > wrong.  And this is IMO a use case for SHA-256 or SHA-512/256
> > > (preferably the latter).  Speed basically doesn't matter here and
> > > Blake2 is both less stable (didn't they slightly change it recently?)
> > > and much less well studied.
> > 
> > We don't prevent ebpf programs being loaded based on the digest but
> > just to uniquely identify loaded programs from user space and match up
> > with their source.
> 
> The commit log talks about using the hash to see if the program has
> already been compiled and JITted.  If that's done, then a collision
> will directly cause the kernel to malfunction.

Yeah, it still shouldn't crash the kernel but it could cause
malfunctions because assumptions are not met from user space thus it
could act in a strange way:

My personal biggest concern is that users of this API will at some
point in time assume this digist is unique (as a key itself for a
hashtables f.e.), while it is actually not (and not enforced so by the
kernel). If you can get an unpriv ebpf program inserted to the kernel
with the same weak hash, a controller daemon could pick it up and bind
it to another ebpf hook, probably outside of the unpriv realm the user
was in before. Only the sorting matters, which might be unstable and is
not guaranteed by anything in most hash table based data structures.

The API seems flawed to me.

> > > My inclination would have been to seed them with something that isn't
> > > exposed to userspace for the precise reason that it would prevent user
> > > code from making assumptions about what's in the hash.  But if there's
> > > a use case for why user code needs to be able to calculate the hash on
> > > its own, then that's fine.  But perhaps the actual fdinfo string
> > > should be "sha256:abcd1234..." to give some flexibility down the road.

To add to this, I am very much in favor of that. Right now it doesn't
have a name because it is a custom algorithm. ;)

> > > 
> > > Also:
> > > 
> > > +   result = (__force __be32 *)fp->digest;
> > > +   for (i = 0; i < SHA_DIGEST_WORDS; i++)
> > > +   result[i] = cpu_to_be32(fp->digest[i]);
> > > 
> > > Everyone, please, please, please don't open-code crypto primitives.
> > > Is this and the code above it even correct?  It might be but on a very
> > > brief glance it looks wrong to me.  If you're doing this to avoid
> > > depending on crypto, then fix crypto so you can pull in the algorithm
> > > without pulling in the whole crypto core.
> > 
> > The hashing is not a proper sha1 neither, unfortunately. I think that
> > is why it will have a custom implementation in iproute2?
> 
> Putting on crypto hat:
> 
> NAK NAK NAK NAK NAK.  "The Linux kernel invented a new primitive in
> 2016 when people know better and is going to handle it by porting that
> new primitive to userspace" is not a particularly good argument.
> 
> Okay, crypto hack back off.
> 
> > 
> > I wondered if bpf program loading should have used the module loading
> > infrastructure from the beginning...
> 
> That would be way too complicated and would be nasty for the unprivileged 
> cases.

I was more or less just thinking about using the syscalls and user
space representation not the generic infrastructure, as it is anyway
too much concerned with real kernel modules (would probably also solve
your cgroup-ebpf thread, as it can be passed by unique name or name and
hash ;) ). Anyway...

> > > At the very least, there should be a separate function that calculates
> > > the hash of a buffer and that function should explicitly run itself
> > > against test vectors of various lengths to make sure that it's
> > > calculating what it claims to be calculating.  And it doesn't look
> > > like the userspace code has landed, so, if this thing isn't
> > > calculating SHA1 correctly, it's plausible that no one has noticed.
> > 
> > I hope this was known from the beginning, this is not sha1 unfortunately.
> > 
> > But ebpf elf programs also need preprocessing to get rid of some
> > embedded load-depending data, so maybe it was considered to be just
> > enough?
> 
> I suspect it was actually an accident.

Maybe, I don't know.

Bye,
Hannes

Re: [PATCHSET v4] blk-mq-scheduling framework

[Bart Van Assche]

On Thu, 2016-12-22 at 09:12 -0800, Omar Sandoval wrote:
> On Thu, Dec 22, 2016 at 04:57:36PM +, Bart Van Assche wrote:
> > On Thu, 2016-12-22 at 08:52 -0800, Omar Sandoval wrote:
> > > This approach occurred to us, but we couldn't figure out a way to make
> > > blk_mq_tag_to_rq() work with it. From skimming over the patches, I
> > > didn't see a solution to that problem.
> > 
> > Can you clarify your comment? Since my patches initialize both tags->rqs[]
> > and sched_tags->rqs[] the function blk_mq_tag_to_rq() should still work.
> 
> Sorry, you're right, it does work, but tags->rqs[] ends up being the
> extra lookup table. I suspect that the runtime overhead of keeping that
> up to date could be worse than copying the rq fields if you have lots of
> CPUs but only one hardware queue.

Hello Omar,

I'm not sure that anything can be done if the number of CPUs that is submitting
I/O is large compared to the queue depth so I don't think we should spend our
time on that case. If the queue depth is large enough then the sbitmap code will
allocate tags such that different CPUs use different rqs[] elements.

The advantages of the approach I proposed are such that I am convinced that is
what we should start from and address contention on the tags->rqs[] array if it
measurements show that it is necessary to address it.

Bart.

Re: [PATCHSET v4] blk-mq-scheduling framework

[Bart Van Assche]

On Thu, 2016-12-22 at 09:12 -0800, Omar Sandoval wrote:
> On Thu, Dec 22, 2016 at 04:57:36PM +, Bart Van Assche wrote:
> > On Thu, 2016-12-22 at 08:52 -0800, Omar Sandoval wrote:
> > > This approach occurred to us, but we couldn't figure out a way to make
> > > blk_mq_tag_to_rq() work with it. From skimming over the patches, I
> > > didn't see a solution to that problem.
> > 
> > Can you clarify your comment? Since my patches initialize both tags->rqs[]
> > and sched_tags->rqs[] the function blk_mq_tag_to_rq() should still work.
> 
> Sorry, you're right, it does work, but tags->rqs[] ends up being the
> extra lookup table. I suspect that the runtime overhead of keeping that
> up to date could be worse than copying the rq fields if you have lots of
> CPUs but only one hardware queue.

Hello Omar,

I'm not sure that anything can be done if the number of CPUs that is submitting
I/O is large compared to the queue depth so I don't think we should spend our
time on that case. If the queue depth is large enough then the sbitmap code will
allocate tags such that different CPUs use different rqs[] elements.

The advantages of the approach I proposed are such that I am convinced that is
what we should start from and address contention on the tags->rqs[] array if it
measurements show that it is necessary to address it.

Bart.

[GIT PULL] LED maintainer's email update

[Jacek Anaszewski]

Hi Linus,

   
Please pull a single patch that advertises a change of my email address.
This pull request is based on my previous one that has been already merged.

The following changes since commit 44b3e31d540e917a4d2292b902ade63fa1748d9a:

  leds: pca955x: Add ACPI support (2016-12-02 09:31:50 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git 
tags/leds_for_4.10_email_update

for you to fetch changes up to 305335b9079426e4619de175ea8d35c669426567:

  MAINTAINERS: Update Jacek Anaszewski's email address (2016-12-22 18:03:25 
+0100)

Thanks,
Jacek Anaszewski

Jacek Anaszewski (1):
  MAINTAINERS: Update Jacek Anaszewski's email address

 MAINTAINERS | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

[GIT PULL] LED maintainer's email update

[Jacek Anaszewski]

Hi Linus,

   
Please pull a single patch that advertises a change of my email address.
This pull request is based on my previous one that has been already merged.

The following changes since commit 44b3e31d540e917a4d2292b902ade63fa1748d9a:

  leds: pca955x: Add ACPI support (2016-12-02 09:31:50 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git 
tags/leds_for_4.10_email_update

for you to fetch changes up to 305335b9079426e4619de175ea8d35c669426567:

  MAINTAINERS: Update Jacek Anaszewski's email address (2016-12-22 18:03:25 
+0100)

Thanks,
Jacek Anaszewski

Jacek Anaszewski (1):
  MAINTAINERS: Update Jacek Anaszewski's email address

 MAINTAINERS | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time

[Greg KH]

On Thu, Dec 22, 2016 at 04:45:45PM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 22 Dec 2016, Guenter Roeck wrote:
> 
> > On 12/22/2016 04:29 AM, Julia Lawall wrote:
> > >
> > >
> > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > >
> > > > Hi Julia,
> > > >
> > > > On Wed, Dec 21, 2016 at 08:39:38PM +0100, Julia Lawall wrote:
> > > > >
> > > > >
> > > > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > > > >
> > > > > > Hi Julia,
> > > > > >
> > > > > > On Wed, Dec 21, 2016 at 03:05:37PM +0100, Julia Lawall wrote:
> > > > > > > A solution is below: the semantic patch, an explanation of the
> > > > > > > semantic
> > > > > > > patch, and the results.  I have only tried to compile the results
> > > > > > > (make
> > > > > > > drivers/hwmon/).  Two affected files were not considered for
> > > > > > > compilation:
> > > > > > >
> > > > > > > drivers/hwmon/vexpress-hwmon.o
> > > > > > > drivers/hwmon/jz4740-hwmon.o
> > > >
> > > > I compile tested those two patches. If possible please drop
> > > > vexpress-hwmon.c
> > > > from the patch series; the changes in that file don't add any value.
> > > >
> > > > I compile tested all files, and reviewed the patch. It all looks good.
> > > > Please submit the series.
> > > >
> > > > Again, thanks a lot for your help!
> > >
> > > I have sent the patches.  I adjusted the semantic patch so that the
> > > indentation of function parameters/arguments would only change if the
> > > length of the function name changes.
> > >
> > > Do you think this could be of more general interest in the Linux kernel?
> > > Since the semantic patch works pretty well, I could add it to the
> > > scripts/coccinelle directory?  Previously, however, I got some negative
> > > feedback about this change, because people felt that the new names hid the
> > > actual behavior, so I didn't pursue it.
> > >
> >
> > I do think it would add a lot of value, if for nothing else as an excellent
> > example
> > of what can be done with coccinelle.
> >
> > I actually liked the name changes. I think it is a good idea if the function
> > name
> > reflects the sysfs attribute it serves (isn't that exactly what it does, ie
> > its
> > behavior ?). But, as you have experienced, some people inadvertently did not
> > like
> > it. Given that, I am not sure if it is worth adding it to the kernel source
> > tree.
> > Maybe you could submit it as RFC so it is at least on record.
> >
> > Anyway, for SENSOR_DEVICE_ATTR(), I'll have to be a bit more flexible since
> > the function _will_ be reused. I'll need something like
> > SENSOR_DEVICE_ATTR_{RO,RW,WO}(attr, func, param)
> 
> Chosen at random,
> 
> static SENSOR_DEVICE_ATTR_2(sf2_point4_fan1, S_IRUGO | S_IWUSR,
> show_sf2_point, store_sf2_point, 4, 1);
> 
> should become
> 
> static SENSOR_DEVICE_ATTR_2_RW(sf2_point4_fan1, sf2_point, 4, 1); ?
> 
> And the functions should be renamed with show and store at the end?
> 
> > Maybe Greg would be open to something like
> > DEVICE_ATTR_FUNC_{RO,RW,WO}(attr,func)
> > to accommodate the "I want my own function name" crowd ? That would also 
> > solve
> > the case where the function is reused for multiple attributes.
> 
> Actually, it was the DEVICE_ATTR_{RO,RW,WO} that wasn't liked.  It doesn't
> show the exact permission numbers.  The fact that not all DEVICE_ATTR uses
> can be changed due to function reuse is awkward, though.  Greg, do you
> have any thoughts about that?

I think the large majority can, and should, be switched to use the
RO,RW,WO variants.  Developers should never be having to specify sysfs
file permissions if at all possible, as they get them wrong too many
times, as we have found out in the past...

> Currently, there are around 1100 calls to DEVICE_ATTR_{RO,RW,WO}.

Those are fine, why do people object to them?

Yes, reusing the function names is not possible, but note, that is a
usually a rare case, the "normal" case for a sysfs attribute can use
these macros just fine.

thanks,

greg k-h

Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time

[Greg KH]

On Thu, Dec 22, 2016 at 04:45:45PM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 22 Dec 2016, Guenter Roeck wrote:
> 
> > On 12/22/2016 04:29 AM, Julia Lawall wrote:
> > >
> > >
> > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > >
> > > > Hi Julia,
> > > >
> > > > On Wed, Dec 21, 2016 at 08:39:38PM +0100, Julia Lawall wrote:
> > > > >
> > > > >
> > > > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > > > >
> > > > > > Hi Julia,
> > > > > >
> > > > > > On Wed, Dec 21, 2016 at 03:05:37PM +0100, Julia Lawall wrote:
> > > > > > > A solution is below: the semantic patch, an explanation of the
> > > > > > > semantic
> > > > > > > patch, and the results.  I have only tried to compile the results
> > > > > > > (make
> > > > > > > drivers/hwmon/).  Two affected files were not considered for
> > > > > > > compilation:
> > > > > > >
> > > > > > > drivers/hwmon/vexpress-hwmon.o
> > > > > > > drivers/hwmon/jz4740-hwmon.o
> > > >
> > > > I compile tested those two patches. If possible please drop
> > > > vexpress-hwmon.c
> > > > from the patch series; the changes in that file don't add any value.
> > > >
> > > > I compile tested all files, and reviewed the patch. It all looks good.
> > > > Please submit the series.
> > > >
> > > > Again, thanks a lot for your help!
> > >
> > > I have sent the patches.  I adjusted the semantic patch so that the
> > > indentation of function parameters/arguments would only change if the
> > > length of the function name changes.
> > >
> > > Do you think this could be of more general interest in the Linux kernel?
> > > Since the semantic patch works pretty well, I could add it to the
> > > scripts/coccinelle directory?  Previously, however, I got some negative
> > > feedback about this change, because people felt that the new names hid the
> > > actual behavior, so I didn't pursue it.
> > >
> >
> > I do think it would add a lot of value, if for nothing else as an excellent
> > example
> > of what can be done with coccinelle.
> >
> > I actually liked the name changes. I think it is a good idea if the function
> > name
> > reflects the sysfs attribute it serves (isn't that exactly what it does, ie
> > its
> > behavior ?). But, as you have experienced, some people inadvertently did not
> > like
> > it. Given that, I am not sure if it is worth adding it to the kernel source
> > tree.
> > Maybe you could submit it as RFC so it is at least on record.
> >
> > Anyway, for SENSOR_DEVICE_ATTR(), I'll have to be a bit more flexible since
> > the function _will_ be reused. I'll need something like
> > SENSOR_DEVICE_ATTR_{RO,RW,WO}(attr, func, param)
> 
> Chosen at random,
> 
> static SENSOR_DEVICE_ATTR_2(sf2_point4_fan1, S_IRUGO | S_IWUSR,
> show_sf2_point, store_sf2_point, 4, 1);
> 
> should become
> 
> static SENSOR_DEVICE_ATTR_2_RW(sf2_point4_fan1, sf2_point, 4, 1); ?
> 
> And the functions should be renamed with show and store at the end?
> 
> > Maybe Greg would be open to something like
> > DEVICE_ATTR_FUNC_{RO,RW,WO}(attr,func)
> > to accommodate the "I want my own function name" crowd ? That would also 
> > solve
> > the case where the function is reused for multiple attributes.
> 
> Actually, it was the DEVICE_ATTR_{RO,RW,WO} that wasn't liked.  It doesn't
> show the exact permission numbers.  The fact that not all DEVICE_ATTR uses
> can be changed due to function reuse is awkward, though.  Greg, do you
> have any thoughts about that?

I think the large majority can, and should, be switched to use the
RO,RW,WO variants.  Developers should never be having to specify sysfs
file permissions if at all possible, as they get them wrong too many
times, as we have found out in the past...

> Currently, there are around 1100 calls to DEVICE_ATTR_{RO,RW,WO}.

Those are fine, why do people object to them?

Yes, reusing the function names is not possible, but note, that is a
usually a rare case, the "normal" case for a sysfs attribute can use
these macros just fine.

thanks,

greg k-h

Assalamu`Alaikum.

[mohammad ouattara]

Dear Sir/Madam.

Assalamu`Alaikum.

I am Dr mohammad ouattara, I have  ($10.6 Million us dollars) to transfer into 
your account,

I will send you more details about this deal and the procedures to follow when 
I receive a positive response from you, 

Have a great day,
Dr mohammad ouattara.

Re: [PATCH 3/3] lib: Update LZ4 compressor module based on LZ4 v1.7.2.

[Greg KH]

On Wed, Dec 21, 2016 at 09:04:02PM +0100, Sven Schmidt wrote:
> On 12/20/2016 08:52 PM, Joe Perches wrote:
> > On Tue, 2016-12-20 at 19:53 +0100, Sven Schmidt wrote:
> >> This patch updates LZ4 kernel module to LZ4 v1.7.2 by Yann Collet.
> >> The kernel module is inspired by the previous work by Chanho Min.
> >> The updated LZ4 module will not break existing code since there were alias
> >> methods added to ensure backwards compatibility.
> >
> >[]
> >
> >> diff --git a/include/linux/lz4.h b/include/linux/lz4.h
> > []
> >> @@ -1,87 +1,218 @@
> >>  #ifndef __LZ4_H__
> >>  #define __LZ4_H__
> >> +
> >>  /*
> >>   * LZ4 Kernel Interface
> >>   *
> >> - * Copyright (C) 2013, LG Electronics, Kyungsik Lee 
> >> + * Copyright (C) 2016, Sven Schmidt <4ssch...@informatik.uni-hamburg.de>
> >
> > Deleting copyright notices is poor form and shouldn't be done.
> >
> > I didn't look at the rest
> >
> 
> Thanks Joe for your time. If you would take a look at the rest, you may notice
> that I changed almost the whole file. That's why I also changed the copyright 
> note.
> If you think I should keep the original note, I will do that.

You have to, please consult a lawyer if you have questions about
copyright issues.  Or better yet, there's a free presentation/course
online about copyright on the linuxfoundation.org website somewhere that
should answer all of your questions here.  I recommend taking a few
minutes and going through that.

thanks,

greg k-h

Assalamu`Alaikum.

[mohammad ouattara]

Dear Sir/Madam.

Assalamu`Alaikum.

I am Dr mohammad ouattara, I have  ($10.6 Million us dollars) to transfer into 
your account,

I will send you more details about this deal and the procedures to follow when 
I receive a positive response from you, 

Have a great day,
Dr mohammad ouattara.

Re: [PATCH 3/3] lib: Update LZ4 compressor module based on LZ4 v1.7.2.

[Greg KH]

On Wed, Dec 21, 2016 at 09:04:02PM +0100, Sven Schmidt wrote:
> On 12/20/2016 08:52 PM, Joe Perches wrote:
> > On Tue, 2016-12-20 at 19:53 +0100, Sven Schmidt wrote:
> >> This patch updates LZ4 kernel module to LZ4 v1.7.2 by Yann Collet.
> >> The kernel module is inspired by the previous work by Chanho Min.
> >> The updated LZ4 module will not break existing code since there were alias
> >> methods added to ensure backwards compatibility.
> >
> >[]
> >
> >> diff --git a/include/linux/lz4.h b/include/linux/lz4.h
> > []
> >> @@ -1,87 +1,218 @@
> >>  #ifndef __LZ4_H__
> >>  #define __LZ4_H__
> >> +
> >>  /*
> >>   * LZ4 Kernel Interface
> >>   *
> >> - * Copyright (C) 2013, LG Electronics, Kyungsik Lee 
> >> + * Copyright (C) 2016, Sven Schmidt <4ssch...@informatik.uni-hamburg.de>
> >
> > Deleting copyright notices is poor form and shouldn't be done.
> >
> > I didn't look at the rest
> >
> 
> Thanks Joe for your time. If you would take a look at the rest, you may notice
> that I changed almost the whole file. That's why I also changed the copyright 
> note.
> If you think I should keep the original note, I will do that.

You have to, please consult a lawyer if you have questions about
copyright issues.  Or better yet, there's a free presentation/course
online about copyright on the linuxfoundation.org website somewhere that
should answer all of your questions here.  I recommend taking a few
minutes and going through that.

thanks,

greg k-h

Re: [PATCH 0/3] Update LZ4 compressor module

[Greg KH]

On Tue, Dec 20, 2016 at 07:53:09PM +0100, Sven Schmidt wrote:
> 
> This patchset is for updating the LZ4 compression module to a version based
> on LZ4 v1.7.2 allowing to use the fast compression algorithm aka LZ4 fast
> which provides an "acceleration" parameter as a tradeoff between
> compression ratio and compression speed.

But why do this?

> We will use LZ4 fast in order to support compression in lustre. LZ4 fast 
> empowers
> us to do client-side as well as server-side compression/decompression while
> being able to provide appropriate parameters to enable users to tune lustre's
> behaviour to obtain the best performance/compression/etc. on their behalf
> (adapative compression).

We don't care about lustre, especially as it is not merged into the main
portion of the kernel tree :)

Seriously, work on fixing up the known issues in lustre before adding
additional features, I've only been saying this for a few _years_ now...

> Also, it will be useful for other users of LZ4 compression,
> as with LZ4 fast it is possible to enable applications to use fast and/or high
> compression depending of the usecase. E.g. a developer can use very
> high compression (low acceleration) for sending data over a network with
> limited rate of transmission or he trades the compression ratio for higher
> compression speed.

This whole patch series is broken, always test-build your code, there's
nothing we could do with these patches even if we wanted to :(

greg k-h

Re: [PATCH 0/3] Update LZ4 compressor module

[Greg KH]

On Tue, Dec 20, 2016 at 07:53:09PM +0100, Sven Schmidt wrote:
> 
> This patchset is for updating the LZ4 compression module to a version based
> on LZ4 v1.7.2 allowing to use the fast compression algorithm aka LZ4 fast
> which provides an "acceleration" parameter as a tradeoff between
> compression ratio and compression speed.

But why do this?

> We will use LZ4 fast in order to support compression in lustre. LZ4 fast 
> empowers
> us to do client-side as well as server-side compression/decompression while
> being able to provide appropriate parameters to enable users to tune lustre's
> behaviour to obtain the best performance/compression/etc. on their behalf
> (adapative compression).

We don't care about lustre, especially as it is not merged into the main
portion of the kernel tree :)

Seriously, work on fixing up the known issues in lustre before adding
additional features, I've only been saying this for a few _years_ now...

> Also, it will be useful for other users of LZ4 compression,
> as with LZ4 fast it is possible to enable applications to use fast and/or high
> compression depending of the usecase. E.g. a developer can use very
> high compression (low acceleration) for sending data over a network with
> limited rate of transmission or he trades the compression ratio for higher
> compression speed.

This whole patch series is broken, always test-build your code, there's
nothing we could do with these patches even if we wanted to :(

greg k-h

[PATCH 9/8] cgroup: fix RCU related sparse warnings

[Tejun Heo]

>From 9ca76e7e3cb885bc5701e9b1fe90c44f4aef123a Mon Sep 17 00:00:00 2001
From: Tejun Heo 
Date: Thu, 22 Dec 2016 12:28:01 -0500

kn->priv which is a void * is used as a RCU pointer by cgroup.  When
dereferencing it, it was passing kn->priv to rcu_derefreence() without
casting it into a RCU pointer triggering address space mismatch
warning from sparse.  Fix them.

Signed-off-by: Tejun Heo 
Reported-by: Fengguang Wu 
---
 kernel/cgroup/cgroup-v1.c | 2 +-
 kernel/cgroup/cgroup.c| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index 37be09e..465b101 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -694,7 +694,7 @@ int cgroupstats_build(struct cgroupstats *stats, struct 
dentry *dentry)
 * @kn->priv is RCU safe.  Let's do the RCU dancing.
 */
rcu_read_lock();
-   cgrp = rcu_dereference(kn->priv);
+   cgrp = rcu_dereference(*(void __rcu __force **)>priv);
if (!cgrp || cgroup_is_dead(cgrp)) {
rcu_read_unlock();
mutex_unlock(_mutex);
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index b6b9068..d9d82e9 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -4932,7 +4932,7 @@ struct cgroup_subsys_state 
*css_tryget_online_from_dir(struct dentry *dentry,
 * have been or be removed at any point.  @kn->priv is RCU
 * protected for this access.  See css_release_work_fn() for details.
 */
-   cgrp = rcu_dereference(kn->priv);
+   cgrp = rcu_dereference(*(void __rcu __force **)>priv);
if (cgrp)
css = cgroup_css(cgrp, ss);
 
-- 
2.9.3

[PATCH 9/8] cgroup: fix RCU related sparse warnings

[Tejun Heo]

>From 9ca76e7e3cb885bc5701e9b1fe90c44f4aef123a Mon Sep 17 00:00:00 2001
From: Tejun Heo 
Date: Thu, 22 Dec 2016 12:28:01 -0500

kn->priv which is a void * is used as a RCU pointer by cgroup.  When
dereferencing it, it was passing kn->priv to rcu_derefreence() without
casting it into a RCU pointer triggering address space mismatch
warning from sparse.  Fix them.

Signed-off-by: Tejun Heo 
Reported-by: Fengguang Wu 
---
 kernel/cgroup/cgroup-v1.c | 2 +-
 kernel/cgroup/cgroup.c| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index 37be09e..465b101 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -694,7 +694,7 @@ int cgroupstats_build(struct cgroupstats *stats, struct 
dentry *dentry)
 * @kn->priv is RCU safe.  Let's do the RCU dancing.
 */
rcu_read_lock();
-   cgrp = rcu_dereference(kn->priv);
+   cgrp = rcu_dereference(*(void __rcu __force **)>priv);
if (!cgrp || cgroup_is_dead(cgrp)) {
rcu_read_unlock();
mutex_unlock(_mutex);
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index b6b9068..d9d82e9 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -4932,7 +4932,7 @@ struct cgroup_subsys_state 
*css_tryget_online_from_dir(struct dentry *dentry,
 * have been or be removed at any point.  @kn->priv is RCU
 * protected for this access.  See css_release_work_fn() for details.
 */
-   cgrp = rcu_dereference(kn->priv);
+   cgrp = rcu_dereference(*(void __rcu __force **)>priv);
if (cgrp)
css = cgroup_css(cgrp, ss);
 
-- 
2.9.3

Re: [PATCH v2 REGRESSION RESEND] usb: ohci-at91: use descriptor-based gpio APIs correctly

[Greg Kroah-Hartman]

On Thu, Dec 22, 2016 at 08:43:55AM +0100, Peter Rosin wrote:
> The gpiod_get* function family does not want the -gpio suffix.
> Use devm_gpiod_get_index_optional instead of devm_gpiod_get_optional.
> The descriptor based APIs handle active high/low automatically.
> The vbus-gpios are output, request enable while getting the gpio.
> Don't try to get any vbus-gpios for ports outside num-ports.
> 
> WTF? Big sigh.
> 
> Fixes: 054d4b7b577d ("usb: ohci-at91: Use descriptor-based gpio APIs")
> Signed-off-by: Peter Rosin 
> ---
> 
> Hi!
> 
> Resending this, since the only response I've got is that the merge
> window is open and that this patch has been put on hold due to that.
> But I think this regression (which happend between v4.9 and current
> master) should be fixed before the merge window closes.

I don't merge patches before -rc1 comes out, sorry, people should have
tested linux-next better :)

I'll catch up the first week of January, relax.

greg k-h

Re: [PATCH v2 REGRESSION RESEND] usb: ohci-at91: use descriptor-based gpio APIs correctly

[Greg Kroah-Hartman]

On Thu, Dec 22, 2016 at 08:43:55AM +0100, Peter Rosin wrote:
> The gpiod_get* function family does not want the -gpio suffix.
> Use devm_gpiod_get_index_optional instead of devm_gpiod_get_optional.
> The descriptor based APIs handle active high/low automatically.
> The vbus-gpios are output, request enable while getting the gpio.
> Don't try to get any vbus-gpios for ports outside num-ports.
> 
> WTF? Big sigh.
> 
> Fixes: 054d4b7b577d ("usb: ohci-at91: Use descriptor-based gpio APIs")
> Signed-off-by: Peter Rosin 
> ---
> 
> Hi!
> 
> Resending this, since the only response I've got is that the merge
> window is open and that this patch has been put on hold due to that.
> But I think this regression (which happend between v4.9 and current
> master) should be fixed before the merge window closes.

I don't merge patches before -rc1 comes out, sorry, people should have
tested linux-next better :)

I'll catch up the first week of January, relax.

greg k-h

[PATCH] MAINTAINERS: Update Jacek Anaszewski's email address

[Jacek Anaszewski]

My previous email address is no longer valid.
>From now on, jacek.anaszew...@gmail.com should be used instead.

Signed-off-by: Jacek Anaszewski 
---
 MAINTAINERS | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1cd38a7..b61650d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1719,7 +1719,7 @@ F:drivers/staging/media/platform/s5p-cec/
 
 ARM/SAMSUNG S5P SERIES JPEG CODEC SUPPORT
 M: Andrzej Pietrasiewicz 
-M: Jacek Anaszewski 
+M: Jacek Anaszewski 
 L: linux-arm-ker...@lists.infradead.org
 L: linux-me...@vger.kernel.org
 S: Maintained
@@ -7070,7 +7070,7 @@ F:drivers/scsi/53c700*
 
 LED SUBSYSTEM
 M: Richard Purdie 
-M: Jacek Anaszewski 
+M: Jacek Anaszewski 
 L: linux-l...@vger.kernel.org
 T: git 
git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git
 S: Maintained
-- 
2.1.4

[PATCH] MAINTAINERS: Update Jacek Anaszewski's email address

[Jacek Anaszewski]

My previous email address is no longer valid.
>From now on, jacek.anaszew...@gmail.com should be used instead.

Signed-off-by: Jacek Anaszewski 
---
 MAINTAINERS | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1cd38a7..b61650d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1719,7 +1719,7 @@ F:drivers/staging/media/platform/s5p-cec/
 
 ARM/SAMSUNG S5P SERIES JPEG CODEC SUPPORT
 M: Andrzej Pietrasiewicz 
-M: Jacek Anaszewski 
+M: Jacek Anaszewski 
 L: linux-arm-ker...@lists.infradead.org
 L: linux-me...@vger.kernel.org
 S: Maintained
@@ -7070,7 +7070,7 @@ F:drivers/scsi/53c700*
 
 LED SUBSYSTEM
 M: Richard Purdie 
-M: Jacek Anaszewski 
+M: Jacek Anaszewski 
 L: linux-l...@vger.kernel.org
 T: git 
git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git
 S: Maintained
-- 
2.1.4

Re: BPF hash algo (Re: [kernel-hardening] Re: [PATCH v7 3/6] random: use SipHash in place of MD5)

[Andy Lutomirski]

On Thu, Dec 22, 2016 at 8:59 AM, Hannes Frederic Sowa
 wrote:
> On Thu, 2016-12-22 at 08:07 -0800, Andy Lutomirski wrote:
>> On Thu, Dec 22, 2016 at 7:51 AM, Hannes Frederic Sowa
>>  wrote:
>> > On Thu, 2016-12-22 at 16:41 +0100, Jason A. Donenfeld wrote:
>> > > Hi Hannes,
>> > >
>> > > On Thu, Dec 22, 2016 at 4:33 PM, Hannes Frederic Sowa
>> > >  wrote:
>> > > > IPv6 you cannot touch anymore. The hashing algorithm is part of uAPI.
>> > > > You don't want to give people new IPv6 addresses with the same stable
>> > > > secret (across reboots) after a kernel upgrade. Maybe they lose
>> > > > connectivity then and it is extra work?
>> > >
>> > > Ahh, too bad. So it goes.
>> >
>> > If no other users survive we can put it into the ipv6 module.
>> >
>> > > > The bpf hash stuff can be changed during this merge window, as it is
>> > > > not yet in a released kernel. Albeit I would probably have preferred
>> > > > something like sha256 here, which can be easily replicated by user
>> > > > space tools (minus the problem of patching out references to not
>> > > > hashable data, which must be zeroed).
>> > >
>> > > Oh, interesting, so time is of the essence then. Do you want to handle
>> > > changing the new eBPF code to something not-SHA1 before it's too late,
>> > > as part of a ne
>>
>> w patchset that can fast track itself to David? And
>> > > then I can preserve my large series for the next merge window.
>> >
>> > This algorithm should be a non-seeded algorithm, because the hashes
>> > should be stable and verifiable by user space tooling. Thus this would
>> > need a hashing algorithm that is hardened against pre-image
>> > attacks/collision resistance, which siphash is not. I would prefer some
>> > higher order SHA algorithm for that actually.
>> >
>>
>> You mean:
>>
>> commit 7bd509e311f408f7a5132fcdde2069af65fa05ae
>> Author: Daniel Borkmann 
>> Date:   Sun Dec 4 23:19:41 2016 +0100
>>
>> bpf: add prog_digest and expose it via fdinfo/netlink
>>
>>
>> Yes, please!  This actually matters for security -- imagine a
>> malicious program brute-forcing a collision so that it gets loaded
>> wrong.  And this is IMO a use case for SHA-256 or SHA-512/256
>> (preferably the latter).  Speed basically doesn't matter here and
>> Blake2 is both less stable (didn't they slightly change it recently?)
>> and much less well studied.
>
> We don't prevent ebpf programs being loaded based on the digest but
> just to uniquely identify loaded programs from user space and match up
> with their source.

The commit log talks about using the hash to see if the program has
already been compiled and JITted.  If that's done, then a collision
will directly cause the kernel to malfunction.

>> My inclination would have been to seed them with something that isn't
>> exposed to userspace for the precise reason that it would prevent user
>> code from making assumptions about what's in the hash.  But if there's
>> a use case for why user code needs to be able to calculate the hash on
>> its own, then that's fine.  But perhaps the actual fdinfo string
>> should be "sha256:abcd1234..." to give some flexibility down the road.
>>
>> Also:
>>
>> +   result = (__force __be32 *)fp->digest;
>> +   for (i = 0; i < SHA_DIGEST_WORDS; i++)
>> +   result[i] = cpu_to_be32(fp->digest[i]);
>>
>> Everyone, please, please, please don't open-code crypto primitives.
>> Is this and the code above it even correct?  It might be but on a very
>> brief glance it looks wrong to me.  If you're doing this to avoid
>> depending on crypto, then fix crypto so you can pull in the algorithm
>> without pulling in the whole crypto core.
>
> The hashing is not a proper sha1 neither, unfortunately. I think that
> is why it will have a custom implementation in iproute2?

Putting on crypto hat:

NAK NAK NAK NAK NAK.  "The Linux kernel invented a new primitive in
2016 when people know better and is going to handle it by porting that
new primitive to userspace" is not a particularly good argument.

Okay, crypto hack back off.

>
> I wondered if bpf program loading should have used the module loading
> infrastructure from the beginning...

That would be way too complicated and would be nasty for the unprivileged cases.

>
>> At the very least, there should be a separate function that calculates
>> the hash of a buffer and that function should explicitly run itself
>> against test vectors of various lengths to make sure that it's
>> calculating what it claims to be calculating.  And it doesn't look
>> like the userspace code has landed, so, if this thing isn't
>> calculating SHA1 correctly, it's plausible that no one has noticed.
>
> I hope this was known from the beginning, this is not sha1 unfortunately.
>
> But ebpf elf programs also need preprocessing to get rid of some
> embedded load-depending data, so maybe it was considered to be just
> enough?

I 

Re: BPF hash algo (Re: [kernel-hardening] Re: [PATCH v7 3/6] random: use SipHash in place of MD5)

[Andy Lutomirski]

On Thu, Dec 22, 2016 at 8:59 AM, Hannes Frederic Sowa
 wrote:
> On Thu, 2016-12-22 at 08:07 -0800, Andy Lutomirski wrote:
>> On Thu, Dec 22, 2016 at 7:51 AM, Hannes Frederic Sowa
>>  wrote:
>> > On Thu, 2016-12-22 at 16:41 +0100, Jason A. Donenfeld wrote:
>> > > Hi Hannes,
>> > >
>> > > On Thu, Dec 22, 2016 at 4:33 PM, Hannes Frederic Sowa
>> > >  wrote:
>> > > > IPv6 you cannot touch anymore. The hashing algorithm is part of uAPI.
>> > > > You don't want to give people new IPv6 addresses with the same stable
>> > > > secret (across reboots) after a kernel upgrade. Maybe they lose
>> > > > connectivity then and it is extra work?
>> > >
>> > > Ahh, too bad. So it goes.
>> >
>> > If no other users survive we can put it into the ipv6 module.
>> >
>> > > > The bpf hash stuff can be changed during this merge window, as it is
>> > > > not yet in a released kernel. Albeit I would probably have preferred
>> > > > something like sha256 here, which can be easily replicated by user
>> > > > space tools (minus the problem of patching out references to not
>> > > > hashable data, which must be zeroed).
>> > >
>> > > Oh, interesting, so time is of the essence then. Do you want to handle
>> > > changing the new eBPF code to something not-SHA1 before it's too late,
>> > > as part of a ne
>>
>> w patchset that can fast track itself to David? And
>> > > then I can preserve my large series for the next merge window.
>> >
>> > This algorithm should be a non-seeded algorithm, because the hashes
>> > should be stable and verifiable by user space tooling. Thus this would
>> > need a hashing algorithm that is hardened against pre-image
>> > attacks/collision resistance, which siphash is not. I would prefer some
>> > higher order SHA algorithm for that actually.
>> >
>>
>> You mean:
>>
>> commit 7bd509e311f408f7a5132fcdde2069af65fa05ae
>> Author: Daniel Borkmann 
>> Date:   Sun Dec 4 23:19:41 2016 +0100
>>
>> bpf: add prog_digest and expose it via fdinfo/netlink
>>
>>
>> Yes, please!  This actually matters for security -- imagine a
>> malicious program brute-forcing a collision so that it gets loaded
>> wrong.  And this is IMO a use case for SHA-256 or SHA-512/256
>> (preferably the latter).  Speed basically doesn't matter here and
>> Blake2 is both less stable (didn't they slightly change it recently?)
>> and much less well studied.
>
> We don't prevent ebpf programs being loaded based on the digest but
> just to uniquely identify loaded programs from user space and match up
> with their source.

The commit log talks about using the hash to see if the program has
already been compiled and JITted.  If that's done, then a collision
will directly cause the kernel to malfunction.

>> My inclination would have been to seed them with something that isn't
>> exposed to userspace for the precise reason that it would prevent user
>> code from making assumptions about what's in the hash.  But if there's
>> a use case for why user code needs to be able to calculate the hash on
>> its own, then that's fine.  But perhaps the actual fdinfo string
>> should be "sha256:abcd1234..." to give some flexibility down the road.
>>
>> Also:
>>
>> +   result = (__force __be32 *)fp->digest;
>> +   for (i = 0; i < SHA_DIGEST_WORDS; i++)
>> +   result[i] = cpu_to_be32(fp->digest[i]);
>>
>> Everyone, please, please, please don't open-code crypto primitives.
>> Is this and the code above it even correct?  It might be but on a very
>> brief glance it looks wrong to me.  If you're doing this to avoid
>> depending on crypto, then fix crypto so you can pull in the algorithm
>> without pulling in the whole crypto core.
>
> The hashing is not a proper sha1 neither, unfortunately. I think that
> is why it will have a custom implementation in iproute2?

Putting on crypto hat:

NAK NAK NAK NAK NAK.  "The Linux kernel invented a new primitive in
2016 when people know better and is going to handle it by porting that
new primitive to userspace" is not a particularly good argument.

Okay, crypto hack back off.

>
> I wondered if bpf program loading should have used the module loading
> infrastructure from the beginning...

That would be way too complicated and would be nasty for the unprivileged cases.

>
>> At the very least, there should be a separate function that calculates
>> the hash of a buffer and that function should explicitly run itself
>> against test vectors of various lengths to make sure that it's
>> calculating what it claims to be calculating.  And it doesn't look
>> like the userspace code has landed, so, if this thing isn't
>> calculating SHA1 correctly, it's plausible that no one has noticed.
>
> I hope this was known from the beginning, this is not sha1 unfortunately.
>
> But ebpf elf programs also need preprocessing to get rid of some
> embedded load-depending data, so maybe it was considered to be just
> enough?

I suspect it was actually an accident.

Re: [4.10, panic, regression] iscsi: null pointer deref at iscsi_tcp_segment_done+0x20d/0x2e0

[Linus Torvalds]

On Wed, Dec 21, 2016 at 10:28 PM, Dave Chinner  wrote:
>
> This sort of thing is normally indicative of a memory reclaim or
> lock contention problem. Profile showed unusual spinlock contention,
> but then I realised there was only one kswapd thread running.
> Yup, sure enough, it's caused by a major change in memory reclaim
> behaviour:
>
> [0.00] Zone ranges:
> [0.00]   DMA  [mem 0x1000-0x00ff]
> [0.00]   DMA32[mem 0x0100-0x]
> [0.00]   Normal   [mem 0x0001-0x00083fff]
> [0.00] Movable zone start for each node
> [0.00] Early memory node ranges
> [0.00]   node   0: [mem 0x1000-0x0009efff]
> [0.00]   node   0: [mem 0x0010-0xbffdefff]
> [0.00]   node   0: [mem 0x0001-0x0003bfff]
> [0.00]   node   0: [mem 0x0005c000-0x0005]
> [0.00]   node   0: [mem 0x0008-0x00083fff]
> [0.00] Initmem setup node 0 [mem 
> 0x1000-0x00083fff]
>
> the numa=fake=4 CLI option is broken.

Ok, I think that is independent of anything else. Removing block
people and adding the x86 people.

I'm not seeing anything at all that would change the fake numa stuff,
but maybe the cpu hotplug changes?

Thomas/Ingo/Peter - Dave is going away for several months, so you
won't get feedback from him, but can you look at this? Or maybe point
me towards the right people - I'm seeing no possible relevant changes
at all fir x85 numa since 4.9, so it must be some indirect breakage.

Dave is using fake-numa to do performance testing in a VM, and it's a
big deal for the node optimizations for writeback etc. Do you have any
ideas?

Dave, if you're still around, can you send out the kernel config file
you used...

   Linus

Re: [4.10, panic, regression] iscsi: null pointer deref at iscsi_tcp_segment_done+0x20d/0x2e0

[Linus Torvalds]

On Wed, Dec 21, 2016 at 10:28 PM, Dave Chinner  wrote:
>
> This sort of thing is normally indicative of a memory reclaim or
> lock contention problem. Profile showed unusual spinlock contention,
> but then I realised there was only one kswapd thread running.
> Yup, sure enough, it's caused by a major change in memory reclaim
> behaviour:
>
> [0.00] Zone ranges:
> [0.00]   DMA  [mem 0x1000-0x00ff]
> [0.00]   DMA32[mem 0x0100-0x]
> [0.00]   Normal   [mem 0x0001-0x00083fff]
> [0.00] Movable zone start for each node
> [0.00] Early memory node ranges
> [0.00]   node   0: [mem 0x1000-0x0009efff]
> [0.00]   node   0: [mem 0x0010-0xbffdefff]
> [0.00]   node   0: [mem 0x0001-0x0003bfff]
> [0.00]   node   0: [mem 0x0005c000-0x0005]
> [0.00]   node   0: [mem 0x0008-0x00083fff]
> [0.00] Initmem setup node 0 [mem 
> 0x1000-0x00083fff]
>
> the numa=fake=4 CLI option is broken.

Ok, I think that is independent of anything else. Removing block
people and adding the x86 people.

I'm not seeing anything at all that would change the fake numa stuff,
but maybe the cpu hotplug changes?

Thomas/Ingo/Peter - Dave is going away for several months, so you
won't get feedback from him, but can you look at this? Or maybe point
me towards the right people - I'm seeing no possible relevant changes
at all fir x85 numa since 4.9, so it must be some indirect breakage.

Dave is using fake-numa to do performance testing in a VM, and it's a
big deal for the node optimizations for writeback etc. Do you have any
ideas?

Dave, if you're still around, can you send out the kernel config file
you used...

   Linus

[PATCH v2] ARM64: zynqmp: Fix i2c node's compatible string

[mdf]

From: Moritz Fischer 

The Zynq Ultrascale MP uses version 1.4 of the Cadence IP core
which fixes some silicon bugs that needed software workarounds
in Version 1.0 that was used on Zynq systems.

Signed-off-by: Moritz Fischer 
Cc: Michal Simek 
Cc: Sören Brinkmann 
Cc: Rob Herring 
---
Changes from v1:

- Added fall through case to cdns,i2c-r1p10

Cheers,

Moritz

---
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi 
b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 68a90833..ee86d02 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -175,7 +175,7 @@
};
 
i2c0: i2c@ff02 {
-   compatible = "cdns,i2c-r1p10";
+   compatible = "cdns,i2c-r1p14", "cdns,i2c-r1p10";
status = "disabled";
interrupt-parent = <>;
interrupts = <0 17 4>;
@@ -185,7 +185,7 @@
};
 
i2c1: i2c@ff03 {
-   compatible = "cdns,i2c-r1p10";
+   compatible = "cdns,i2c-r1p14", "cdns,i2c-r1p10";
status = "disabled";
interrupt-parent = <>;
interrupts = <0 18 4>;
-- 
2.7.4

[PATCH v2] ARM64: zynqmp: Fix i2c node's compatible string

[mdf]

From: Moritz Fischer 

The Zynq Ultrascale MP uses version 1.4 of the Cadence IP core
which fixes some silicon bugs that needed software workarounds
in Version 1.0 that was used on Zynq systems.

Signed-off-by: Moritz Fischer 
Cc: Michal Simek 
Cc: Sören Brinkmann 
Cc: Rob Herring 
---
Changes from v1:

- Added fall through case to cdns,i2c-r1p10

Cheers,

Moritz

---
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi 
b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 68a90833..ee86d02 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -175,7 +175,7 @@
};
 
i2c0: i2c@ff02 {
-   compatible = "cdns,i2c-r1p10";
+   compatible = "cdns,i2c-r1p14", "cdns,i2c-r1p10";
status = "disabled";
interrupt-parent = <>;
interrupts = <0 17 4>;
@@ -185,7 +185,7 @@
};
 
i2c1: i2c@ff03 {
-   compatible = "cdns,i2c-r1p10";
+   compatible = "cdns,i2c-r1p14", "cdns,i2c-r1p10";
status = "disabled";
interrupt-parent = <>;
interrupts = <0 18 4>;
-- 
2.7.4

Re: [PATCH 2/2] HID: intel-ish-hid: format 32-bit integers with %X

[Srinivas Pandruvada]

On Thu, 2016-12-22 at 11:09 +0100, Nicolas Iooss wrote:
> In ishtp_hid_probe(), use %04X instead of %04hX to format __u32
> values,
> in order to silent a format error reported by clang:
> 
> drivers/hid/intel-ish-hid/ishtp-hid.c:212:3: error: format
> specifies
> type 'unsigned short' but the argument has type '__u32' (aka
> 'unsigned int') [-Werror,-Wformat]
> hid->vendor, hid->product);
> ^~~
> drivers/hid/intel-ish-hid/ishtp-hid.c:212:16: error: format
> specifies type 'unsigned short' but the argument has type '__u32'
> (aka 'unsigned int') [-Werror,-Wformat]
> hid->vendor, hid->product);
>  ^~~~
> 
> Signed-off-by: Nicolas Iooss 
Acked-by: Srinivas Pandruvada 

> ---
>  drivers/hid/intel-ish-hid/ishtp-hid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.c
> b/drivers/hid/intel-ish-hid/ishtp-hid.c
> index 277983aa1d90..cd23903ddcf1 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid.c
> @@ -208,7 +208,7 @@ int ishtp_hid_probe(unsigned int cur_hid_dev,
>   hid->version = le16_to_cpu(ISH_HID_VERSION);
>   hid->vendor = le16_to_cpu(ISH_HID_VENDOR);
>   hid->product = le16_to_cpu(ISH_HID_PRODUCT);
> - snprintf(hid->name, sizeof(hid->name), "%s %04hX:%04hX",
> "hid-ishtp",
> + snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X", "hid-
> ishtp",
>   hid->vendor, hid->product);
>  
>   rv = hid_add_device(hid);

Re: [PATCH 2/2] HID: intel-ish-hid: format 32-bit integers with %X

[Srinivas Pandruvada]

On Thu, 2016-12-22 at 11:09 +0100, Nicolas Iooss wrote:
> In ishtp_hid_probe(), use %04X instead of %04hX to format __u32
> values,
> in order to silent a format error reported by clang:
> 
> drivers/hid/intel-ish-hid/ishtp-hid.c:212:3: error: format
> specifies
> type 'unsigned short' but the argument has type '__u32' (aka
> 'unsigned int') [-Werror,-Wformat]
> hid->vendor, hid->product);
> ^~~
> drivers/hid/intel-ish-hid/ishtp-hid.c:212:16: error: format
> specifies type 'unsigned short' but the argument has type '__u32'
> (aka 'unsigned int') [-Werror,-Wformat]
> hid->vendor, hid->product);
>  ^~~~
> 
> Signed-off-by: Nicolas Iooss 
Acked-by: Srinivas Pandruvada 

> ---
>  drivers/hid/intel-ish-hid/ishtp-hid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.c
> b/drivers/hid/intel-ish-hid/ishtp-hid.c
> index 277983aa1d90..cd23903ddcf1 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid.c
> @@ -208,7 +208,7 @@ int ishtp_hid_probe(unsigned int cur_hid_dev,
>   hid->version = le16_to_cpu(ISH_HID_VERSION);
>   hid->vendor = le16_to_cpu(ISH_HID_VENDOR);
>   hid->product = le16_to_cpu(ISH_HID_PRODUCT);
> - snprintf(hid->name, sizeof(hid->name), "%s %04hX:%04hX",
> "hid-ishtp",
> + snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X", "hid-
> ishtp",
>   hid->vendor, hid->product);
>  
>   rv = hid_add_device(hid);

Re: [PATCH 1/2] HID: intel-ish-hid: add printf attribute to print_log()

[Srinivas Pandruvada]

On Thu, 2016-12-22 at 11:09 +0100, Nicolas Iooss wrote:
> Structure ishtp_device contains a logging function, print_log(),
> which
> formats some of its parameters using vsnprintf(). Add a __printf
> attribute to this function field (and to ish_event_tracer()) in order
> to
> detect at compile time issues related to the printf-like formatting.
> 
> While at it, make format parameter a const pointer as print_log() is
> not
> supposed to modify it.
> 
> Signed-off-by: Nicolas Iooss 
Acked-by: Srinivas Pandruvada 

> ---
>  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 3 ++-
>  drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index 20d647d2dd2c..34c95de6885e 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -47,7 +47,8 @@ MODULE_DEVICE_TABLE(pci, ish_pci_tbl);
>   *
>   * Callback to direct log messages to Linux trace buffers
>   */
> -static void ish_event_tracer(struct ishtp_device *dev, char *format,
> ...)
> +static __printf(2, 3)
> +void ish_event_tracer(struct ishtp_device *dev, const char *format,
> ...)
>  {
>   if (trace_ishtp_dump_enabled()) {
>   va_list args;
> diff --git a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> index a94f9a8a96a0..6a6d927b78b0 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> +++ b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> @@ -238,7 +238,8 @@ struct ishtp_device {
>   uint64_t ishtp_host_dma_rx_buf_phys;
>  
>   /* Dump to trace buffers if enabled*/
> - void (*print_log)(struct ishtp_device *dev, char *format,
> ...);
> + __printf(2, 3) void (*print_log)(struct ishtp_device *dev,
> +  const char *format, ...);
>  
>   /* Debug stats */
>   unsigned intipc_rx_cnt;

Re: [PATCH 1/2] HID: intel-ish-hid: add printf attribute to print_log()

[Srinivas Pandruvada]

On Thu, 2016-12-22 at 11:09 +0100, Nicolas Iooss wrote:
> Structure ishtp_device contains a logging function, print_log(),
> which
> formats some of its parameters using vsnprintf(). Add a __printf
> attribute to this function field (and to ish_event_tracer()) in order
> to
> detect at compile time issues related to the printf-like formatting.
> 
> While at it, make format parameter a const pointer as print_log() is
> not
> supposed to modify it.
> 
> Signed-off-by: Nicolas Iooss 
Acked-by: Srinivas Pandruvada 

> ---
>  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 3 ++-
>  drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index 20d647d2dd2c..34c95de6885e 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -47,7 +47,8 @@ MODULE_DEVICE_TABLE(pci, ish_pci_tbl);
>   *
>   * Callback to direct log messages to Linux trace buffers
>   */
> -static void ish_event_tracer(struct ishtp_device *dev, char *format,
> ...)
> +static __printf(2, 3)
> +void ish_event_tracer(struct ishtp_device *dev, const char *format,
> ...)
>  {
>   if (trace_ishtp_dump_enabled()) {
>   va_list args;
> diff --git a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> index a94f9a8a96a0..6a6d927b78b0 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> +++ b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> @@ -238,7 +238,8 @@ struct ishtp_device {
>   uint64_t ishtp_host_dma_rx_buf_phys;
>  
>   /* Dump to trace buffers if enabled*/
> - void (*print_log)(struct ishtp_device *dev, char *format,
> ...);
> + __printf(2, 3) void (*print_log)(struct ishtp_device *dev,
> +  const char *format, ...);
>  
>   /* Debug stats */
>   unsigned intipc_rx_cnt;

Re: [v1] misc: cb710: core:- Handle return NULL error from pcim_iomap_table

[arvind Yadav]

Ignore this change. Sorry for the noise.

Thanks
Arvind


On Thursday 22 December 2016 05:28 PM, Arvind Yadav wrote:

Here, If pcim_iomap_table will fail. It will return NULL.
Kernel can run into a NULL-pointer dereference.
This error check will avoid NULL pointer dereference.

Signed-off-by: Arvind Yadav 
---
  drivers/misc/cb710/core.c |2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/misc/cb710/core.c b/drivers/misc/cb710/core.c
index fb397e7..7b60019 100644
--- a/drivers/misc/cb710/core.c
+++ b/drivers/misc/cb710/core.c
@@ -248,6 +248,8 @@ static int cb710_probe(struct pci_dev *pdev,
spin_lock_init(>irq_lock);
chip->pdev = pdev;
chip->iobase = pcim_iomap_table(pdev)[0];
+   if (!chip->iobase)
+   return -ENOMEM;
  
  	pci_set_drvdata(pdev, chip);
  

Re: [v1] misc: cb710: core:- Handle return NULL error from pcim_iomap_table

[arvind Yadav]

Ignore this change. Sorry for the noise.

Thanks
Arvind


On Thursday 22 December 2016 05:28 PM, Arvind Yadav wrote:

Here, If pcim_iomap_table will fail. It will return NULL.
Kernel can run into a NULL-pointer dereference.
This error check will avoid NULL pointer dereference.

Signed-off-by: Arvind Yadav 
---
  drivers/misc/cb710/core.c |2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/misc/cb710/core.c b/drivers/misc/cb710/core.c
index fb397e7..7b60019 100644
--- a/drivers/misc/cb710/core.c
+++ b/drivers/misc/cb710/core.c
@@ -248,6 +248,8 @@ static int cb710_probe(struct pci_dev *pdev,
spin_lock_init(>irq_lock);
chip->pdev = pdev;
chip->iobase = pcim_iomap_table(pdev)[0];
+   if (!chip->iobase)
+   return -ENOMEM;
  
  	pci_set_drvdata(pdev, chip);
  

Re: [v1] mmc: host: dw_mmc-pci:- Handle return NULL error from pcim_iomap_table

[arvind Yadav]

Ignore this change. Sorry for noise.

Thanks
-Arvind

On Thursday 22 December 2016 05:36 PM, Arvind Yadav wrote:

Here, If pcim_iomap_table will fail. It will return NULL.
Kernel can run into a NULL-pointer dereference.
This error check will avoid NULL pointer dereference.

Signed-off-by: Arvind Yadav 
---
  drivers/mmc/host/dw_mmc-pci.c |2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc-pci.c b/drivers/mmc/host/dw_mmc-pci.c
index ab82796..f42d302 100644
--- a/drivers/mmc/host/dw_mmc-pci.c
+++ b/drivers/mmc/host/dw_mmc-pci.c
@@ -61,6 +61,8 @@ static int dw_mci_pci_probe(struct pci_dev *pdev,
return ret;
  
  	host->regs = pcim_iomap_table(pdev)[PCI_BAR_NO];

+   if (!host->regs)
+   return -ENOMEM;
  
  	pci_set_master(pdev);
  

Re: [v1] mmc: host: dw_mmc-pci:- Handle return NULL error from pcim_iomap_table

[arvind Yadav]

Ignore this change. Sorry for noise.

Thanks
-Arvind

On Thursday 22 December 2016 05:36 PM, Arvind Yadav wrote:

Here, If pcim_iomap_table will fail. It will return NULL.
Kernel can run into a NULL-pointer dereference.
This error check will avoid NULL pointer dereference.

Signed-off-by: Arvind Yadav 
---
  drivers/mmc/host/dw_mmc-pci.c |2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc-pci.c b/drivers/mmc/host/dw_mmc-pci.c
index ab82796..f42d302 100644
--- a/drivers/mmc/host/dw_mmc-pci.c
+++ b/drivers/mmc/host/dw_mmc-pci.c
@@ -61,6 +61,8 @@ static int dw_mci_pci_probe(struct pci_dev *pdev,
return ret;
  
  	host->regs = pcim_iomap_table(pdev)[PCI_BAR_NO];

+   if (!host->regs)
+   return -ENOMEM;
  
  	pci_set_master(pdev);
  

Re: [PATCHSET v4] blk-mq-scheduling framework

[Omar Sandoval]

On Thu, Dec 22, 2016 at 04:57:36PM +, Bart Van Assche wrote:
> On Thu, 2016-12-22 at 08:52 -0800, Omar Sandoval wrote:
> > This approach occurred to us, but we couldn't figure out a way to make
> > blk_mq_tag_to_rq() work with it. From skimming over the patches, I
> > didn't see a solution to that problem.
> 
> Hello Omar,
> 
> Can you clarify your comment? Since my patches initialize both tags->rqs[]
> and sched_tags->rqs[] the function blk_mq_tag_to_rq() should still work.
> 
> Bart.

Sorry, you're right, it does work, but tags->rqs[] ends up being the
extra lookup table. I suspect that the runtime overhead of keeping that
up to date could be worse than copying the rq fields if you have lots of
CPUs but only one hardware queue.

Re: [PATCHSET v4] blk-mq-scheduling framework

[Omar Sandoval]

On Thu, Dec 22, 2016 at 04:57:36PM +, Bart Van Assche wrote:
> On Thu, 2016-12-22 at 08:52 -0800, Omar Sandoval wrote:
> > This approach occurred to us, but we couldn't figure out a way to make
> > blk_mq_tag_to_rq() work with it. From skimming over the patches, I
> > didn't see a solution to that problem.
> 
> Hello Omar,
> 
> Can you clarify your comment? Since my patches initialize both tags->rqs[]
> and sched_tags->rqs[] the function blk_mq_tag_to_rq() should still work.
> 
> Bart.

Sorry, you're right, it does work, but tags->rqs[] ends up being the
extra lookup table. I suspect that the runtime overhead of keeping that
up to date could be worse than copying the rq fields if you have lots of
CPUs but only one hardware queue.

Re: [v1] i2c: busses: i2c-designware-pcidrv:- Handle return NULL error from pcim_iomap_table

[arvind Yadav]

Yes, It will not fail. Sorry for the noise.

Thanks
-Arvind

On Thursday 22 December 2016 08:05 PM, Andy Shevchenko wrote:

On Thu, 2016-12-22 at 17:09 +0530, Arvind Yadav wrote:

Here, If pcim_iomap_table will fail. It will return NULL.
Kernel can run into a NULL-pointer dereference.
This error check will avoid NULL pointer dereference.

Signed-off-by: Arvind Yadav 
---
  drivers/i2c/busses/i2c-designware-pcidrv.c |4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
b/drivers/i2c/busses/i2c-designware-pcidrv.c
index d6423cf..6a1907d 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -235,6 +235,10 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
dev->controller = controller;
dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
dev->base = pcim_iomap_table(pdev)[0];
+   if (!dev->base) {
+   dev_err(>dev, "I/O map table allocation
failed\n");
+   return -ENOMEM;
+   }

NAK.

Re: [v1] i2c: busses: i2c-designware-pcidrv:- Handle return NULL error from pcim_iomap_table

[arvind Yadav]

Yes, It will not fail. Sorry for the noise.

Thanks
-Arvind

On Thursday 22 December 2016 08:05 PM, Andy Shevchenko wrote:

On Thu, 2016-12-22 at 17:09 +0530, Arvind Yadav wrote:

Here, If pcim_iomap_table will fail. It will return NULL.
Kernel can run into a NULL-pointer dereference.
This error check will avoid NULL pointer dereference.

Signed-off-by: Arvind Yadav 
---
  drivers/i2c/busses/i2c-designware-pcidrv.c |4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
b/drivers/i2c/busses/i2c-designware-pcidrv.c
index d6423cf..6a1907d 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -235,6 +235,10 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
dev->controller = controller;
dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
dev->base = pcim_iomap_table(pdev)[0];
+   if (!dev->base) {
+   dev_err(>dev, "I/O map table allocation
failed\n");
+   return -ENOMEM;
+   }

NAK.

Re: [PATCHv6 6/7] printk: use printk_safe buffers in printk

[Petr Mladek]

On Thu 2016-12-22 14:31:19, Sergey Senozhatsky wrote:
> Hello,
> 
> On (12/21/16 23:36), Sergey Senozhatsky wrote:
> > Use printk_safe per-CPU buffers in printk recursion-prone blocks:
> > -- around logbuf_lock protected sections in vprintk_emit() and
> >console_unlock()
> > -- around down_trylock_console_sem() and up_console_sem()
> > 
> > Note that this solution addresses deadlocks caused by printk()
> > recursive calls only. That is vprintk_emit() and console_unlock().
> 
> several questions.

Good questions!

> so my plan was to introduce printk-safe and to switch vprintk_emit()
> and console_sem related functions (like console_unlock(), etc.) to
> printk-safe first. and switch the remaining logbuf_lock users, like
> devkmsg_open()/syslog_print()/etc, in a followup, pretty much
> mechanical "find logbuf_lock - add printk_safe", patch. but that
> followup patch is bigger than I expected (still mechanical tho);
> so I want to re-group.
> 
> there are
>   9 raw_spin_lock_irq(_lock)
>   7 raw_spin_lock_irqsave(_lock, flags)
> and
>   12 raw_spin_lock_irq(_lock)
>   8 raw_spin_unlock_irqrestore(_lock, flags)

I see.

> wrapping each one of them in printk_safe_enter()/printk_safe_enter_irq()
> and printk_safe_exit()/printk_safe_exit_irq() is a bit boring. so I have
> several options: one of them is to add printk_safe_{enter,exit}_irq() and,
> along with it, a bunch of help macros (to printk.c):
> 
> (questions below)
> 
> /*
>  * Helper macros to lock/unlock logbuf_lock in deadlock safe
>  * manner (logbuf_lock may spin_dump() in lock/unlock).
>  */
> #define lock_logbuf(flags)\
>   do {\
>   printk_safe_enter(flags);   \
>   raw_spin_lock(_lock);\
>   } while (0)

There are many callers. I think that such wrappers make sense.
I would only like to keep naming scheme similar to the classic
locks. I mean:

printk_safe_enter_irq()
printk_safe_exit_irq()

printk_safe_enter_irqsave(flags)
printk_safe_exit_irqrestore(flags)

and

logbuf_lock_irq()
logbuf_unlock_irq()

logbuf_lock_irqsave(flags)
logbuf_lock_irqrestore(flags)

I actually like this change. It makes it clear that the operation
has a side effect (disables/enables irq) which was not visible
from the original name.


Well, I wonder how many times we need to call printk_save_enter/exit
standalone (ouside these macros).

The question is if we really need all the variants of
printk_safe_enter()/exit(). Alternative solution would be
to handle only the printk_context in pritnk_safe_enter()
and make sure that it is called with IRQs disabled.
I mean to define only __printk_safe_enter()/exit()
and do:

#define logbuf_lock_irqsave(flags)  \
do {\
local_irq_save(flags)   \
__printk_safe_enter();  \
raw_spin_lock(_lock);\
} while (0)


> -- the approach
>  another solution? switch those raw_spin_{lock,unlock}_irq to 
> irqsave/irqrestore
>  (?) and use the existing printk_safe_enter()/printk_safe_exit(),
>  so *_irq() versions of lock_logbuf/printk_safe macros will not be needed?

This is bad idea. We should keep the information where the flags need
to be stored and where not. It helps to understand in which context
the function might be called.

Best Regards,
Petr

PS: I still think if we could come with a better name than
printk_safe() but I cannot find one.

Re: [PATCHv6 6/7] printk: use printk_safe buffers in printk

[Petr Mladek]

On Thu 2016-12-22 14:31:19, Sergey Senozhatsky wrote:
> Hello,
> 
> On (12/21/16 23:36), Sergey Senozhatsky wrote:
> > Use printk_safe per-CPU buffers in printk recursion-prone blocks:
> > -- around logbuf_lock protected sections in vprintk_emit() and
> >console_unlock()
> > -- around down_trylock_console_sem() and up_console_sem()
> > 
> > Note that this solution addresses deadlocks caused by printk()
> > recursive calls only. That is vprintk_emit() and console_unlock().
> 
> several questions.

Good questions!

> so my plan was to introduce printk-safe and to switch vprintk_emit()
> and console_sem related functions (like console_unlock(), etc.) to
> printk-safe first. and switch the remaining logbuf_lock users, like
> devkmsg_open()/syslog_print()/etc, in a followup, pretty much
> mechanical "find logbuf_lock - add printk_safe", patch. but that
> followup patch is bigger than I expected (still mechanical tho);
> so I want to re-group.
> 
> there are
>   9 raw_spin_lock_irq(_lock)
>   7 raw_spin_lock_irqsave(_lock, flags)
> and
>   12 raw_spin_lock_irq(_lock)
>   8 raw_spin_unlock_irqrestore(_lock, flags)

I see.

> wrapping each one of them in printk_safe_enter()/printk_safe_enter_irq()
> and printk_safe_exit()/printk_safe_exit_irq() is a bit boring. so I have
> several options: one of them is to add printk_safe_{enter,exit}_irq() and,
> along with it, a bunch of help macros (to printk.c):
> 
> (questions below)
> 
> /*
>  * Helper macros to lock/unlock logbuf_lock in deadlock safe
>  * manner (logbuf_lock may spin_dump() in lock/unlock).
>  */
> #define lock_logbuf(flags)\
>   do {\
>   printk_safe_enter(flags);   \
>   raw_spin_lock(_lock);\
>   } while (0)

There are many callers. I think that such wrappers make sense.
I would only like to keep naming scheme similar to the classic
locks. I mean:

printk_safe_enter_irq()
printk_safe_exit_irq()

printk_safe_enter_irqsave(flags)
printk_safe_exit_irqrestore(flags)

and

logbuf_lock_irq()
logbuf_unlock_irq()

logbuf_lock_irqsave(flags)
logbuf_lock_irqrestore(flags)

I actually like this change. It makes it clear that the operation
has a side effect (disables/enables irq) which was not visible
from the original name.


Well, I wonder how many times we need to call printk_save_enter/exit
standalone (ouside these macros).

The question is if we really need all the variants of
printk_safe_enter()/exit(). Alternative solution would be
to handle only the printk_context in pritnk_safe_enter()
and make sure that it is called with IRQs disabled.
I mean to define only __printk_safe_enter()/exit()
and do:

#define logbuf_lock_irqsave(flags)  \
do {\
local_irq_save(flags)   \
__printk_safe_enter();  \
raw_spin_lock(_lock);\
} while (0)


> -- the approach
>  another solution? switch those raw_spin_{lock,unlock}_irq to 
> irqsave/irqrestore
>  (?) and use the existing printk_safe_enter()/printk_safe_exit(),
>  so *_irq() versions of lock_logbuf/printk_safe macros will not be needed?

This is bad idea. We should keep the information where the flags need
to be stored and where not. It helps to understand in which context
the function might be called.

Best Regards,
Petr

PS: I still think if we could come with a better name than
printk_safe() but I cannot find one.

Re: [PATCH v2] stmmac: CSR clock configuration fix

[Joao Pinto]

Às 4:57 PM de 12/22/2016, Phil Reid escreveu:
> On 22/12/2016 23:47, Joao Pinto wrote:
>>
>> Hello Phil,
>>
>> Às 3:42 PM de 12/22/2016, Phil Reid escreveu:
>>> G'day Joao,
>>>
>>> On 22/12/2016 20:38, Joao Pinto wrote:
 When testing stmmac with my QoS reference design I checked a problem in the
 CSR clock configuration that was impossibilitating the phy discovery, since
 every read operation returned 0x. This patch fixes the issue.

 Signed-off-by: Joao Pinto 
 ---
 changes v1->v2 (David Miller)
 - DWMAC100 and DWMAC1000 csr clocks masks should also be fixed for the 
 patch
 to make sense

  drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c | 2 +-
  drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c  | 2 +-
  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c| 8 
  3 files changed, 6 insertions(+), 6 deletions(-)

 diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
 b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
 index b21d03f..94223c8 100644
 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
 +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
 @@ -539,7 +539,7 @@ struct mac_device_info *dwmac1000_setup(void __iomem
 *ioaddr, int mcbins,
  mac->mii.reg_shift = 6;
  mac->mii.reg_mask = 0x07C0;
  mac->mii.clk_csr_shift = 2;
 -mac->mii.clk_csr_mask = 0xF;
 +mac->mii.clk_csr_mask = GENMASK(4, 2);
>>>
>>> Should this not be GENMASK(5,2)
>>
>> According to Universal MAC databook (valid for MAC100 and MAC1000), we have:
>>
>> Bits: 4:2
>> 000 60-100 MHz clk_csr_i/42
>> 001 100-150 MHz clk_csr_i/62
>> 010 20-35 MHz clk_csr_i/16
>> 011 35-60 MHz clk_csr_i/26
>> 100 150-250 MHz clk_csr_i/102
>> 101 250-300 MHz clk_csr_i/124
>> 110, 111 Reserved
>>
>> So only bits 2, 3 and 4 should be masked.
>>
>> Thanks.
>>
> But this is a change in behaviour from what was there isn't.
> Previous mask was 4 bits. now it's 3.
> 
> And for example the altera socfgpa implementation in the cyclone V has valid 
> values
> for values 0x8-0xf, using bit 5:2.

According to the databook, bit 5 is reserved and RO. When reserved, it is
possible to customize. Is that the case? If there is hardware using the 5th bit
we can put it GENMASK(5, 2) with no problem.

> 
> 
> 
> 
> 

Re: [PATCH v2] stmmac: CSR clock configuration fix

[Joao Pinto]

Às 4:57 PM de 12/22/2016, Phil Reid escreveu:
> On 22/12/2016 23:47, Joao Pinto wrote:
>>
>> Hello Phil,
>>
>> Às 3:42 PM de 12/22/2016, Phil Reid escreveu:
>>> G'day Joao,
>>>
>>> On 22/12/2016 20:38, Joao Pinto wrote:
 When testing stmmac with my QoS reference design I checked a problem in the
 CSR clock configuration that was impossibilitating the phy discovery, since
 every read operation returned 0x. This patch fixes the issue.

 Signed-off-by: Joao Pinto 
 ---
 changes v1->v2 (David Miller)
 - DWMAC100 and DWMAC1000 csr clocks masks should also be fixed for the 
 patch
 to make sense

  drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c | 2 +-
  drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c  | 2 +-
  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c| 8 
  3 files changed, 6 insertions(+), 6 deletions(-)

 diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
 b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
 index b21d03f..94223c8 100644
 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
 +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
 @@ -539,7 +539,7 @@ struct mac_device_info *dwmac1000_setup(void __iomem
 *ioaddr, int mcbins,
  mac->mii.reg_shift = 6;
  mac->mii.reg_mask = 0x07C0;
  mac->mii.clk_csr_shift = 2;
 -mac->mii.clk_csr_mask = 0xF;
 +mac->mii.clk_csr_mask = GENMASK(4, 2);
>>>
>>> Should this not be GENMASK(5,2)
>>
>> According to Universal MAC databook (valid for MAC100 and MAC1000), we have:
>>
>> Bits: 4:2
>> 000 60-100 MHz clk_csr_i/42
>> 001 100-150 MHz clk_csr_i/62
>> 010 20-35 MHz clk_csr_i/16
>> 011 35-60 MHz clk_csr_i/26
>> 100 150-250 MHz clk_csr_i/102
>> 101 250-300 MHz clk_csr_i/124
>> 110, 111 Reserved
>>
>> So only bits 2, 3 and 4 should be masked.
>>
>> Thanks.
>>
> But this is a change in behaviour from what was there isn't.
> Previous mask was 4 bits. now it's 3.
> 
> And for example the altera socfgpa implementation in the cyclone V has valid 
> values
> for values 0x8-0xf, using bit 5:2.

According to the databook, bit 5 is reserved and RO. When reserved, it is
possible to customize. Is that the case? If there is hardware using the 5th bit
we can put it GENMASK(5, 2) with no problem.

> 
> 
> 
> 
> 

[PATCH 0/2] sched: Introduce rcuwait

[Davidlohr Bueso]

Hi,

Here's an updated version of the pcpu rwsem writer wait/wake changes
with the abstractions wanted by Oleg. Patch 1 adds rcuwait (for a lack
of better name), and patch 2 trivially makes use of it. 

Has survived torture testing, which is actually very handy in this case
particularly dealing with equal amount of reader and writer threads.

Thanks.

Davidlohr Bueso (2):
  sched: Introduce rcuwait machinery
  locking/percpu-rwsem: Replace waitqueue with rcuwait

 include/linux/percpu-rwsem.h  |  8 +++---
 include/linux/rcuwait.h   | 63 +++
 kernel/exit.c | 29 
 kernel/locking/percpu-rwsem.c |  7 +++--
 4 files changed, 99 insertions(+), 8 deletions(-)
 create mode 100644 include/linux/rcuwait.h

-- 
2.6.6

[PATCH] staging: rtl8188eu: In core directory, fixed 'missing a balnk line after declarations' warnings.

[Yamanappagouda Patil]

Fixed checkpatch.pl warnings in rtl8188eu/core directory.

Signed-off-by: Yamanappagouda Patil 
---
 drivers/staging/rtl8188eu/core/rtw_efuse.c |  4 
 drivers/staging/rtl8188eu/core/rtw_ieee80211.c |  5 -
 drivers/staging/rtl8188eu/core/rtw_led.c   |  1 +
 drivers/staging/rtl8188eu/core/rtw_mlme.c  |  3 +++
 drivers/staging/rtl8188eu/core/rtw_mlme_ext.c  | 10 +-
 drivers/staging/rtl8188eu/core/rtw_pwrctrl.c   |  1 +
 drivers/staging/rtl8188eu/core/rtw_recv.c  |  9 -
 drivers/staging/rtl8188eu/core/rtw_security.c  | 18 ++
 drivers/staging/rtl8188eu/core/rtw_sta_mgt.c   |  1 +
 drivers/staging/rtl8188eu/core/rtw_wlan_util.c |  3 +++
 drivers/staging/rtl8188eu/core/rtw_xmit.c  |  3 +++
 11 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_efuse.c 
b/drivers/staging/rtl8188eu/core/rtw_efuse.c
index 16cc770..5dd9b24 100644
--- a/drivers/staging/rtl8188eu/core/rtw_efuse.c
+++ b/drivers/staging/rtl8188eu/core/rtw_efuse.c
@@ -253,6 +253,7 @@ static void efuse_read_phymap_from_txpktbuf(
u8 lenc[2];
u16 lenbak, aaabak;
u16 aaa;
+
lenc[0] = usb_read8(adapter, REG_PKTBUF_DBG_DATA_L);
lenc[1] = usb_read8(adapter, REG_PKTBUF_DBG_DATA_L+1);
 
@@ -569,6 +570,7 @@ static bool hal_EfusePgPacketWrite2ByteHeader(struct 
adapter *pAdapter, u8 efuse
continue;
} else if (pg_header != tmp_header) {   /* offset PG 
fail */
struct pgpktfixPkt;
+
fixPkt.offset = ((pg_header_temp & 0xE0) >> 5) 
| ((tmp_header & 0xF0) >> 1);
fixPkt.word_en = tmp_header & 0x0F;
fixPkt.word_cnts = 
Efuse_CalculateWordCnts(fixPkt.word_en);
@@ -611,6 +613,7 @@ static bool hal_EfusePgPacketWrite1ByteHeader(struct 
adapter *pAdapter, u8 efuse
bRet = true;
} else {
struct pgpktfixPkt;
+
fixPkt.offset = (tmp_header>>4) & 0x0F;
fixPkt.word_en = tmp_header & 0x0F;
fixPkt.word_cnts = Efuse_CalculateWordCnts(fixPkt.word_en);
@@ -819,6 +822,7 @@ bool Efuse_PgPacketWrite(struct adapter *pAdapter, u8 
offset, u8 word_en, u8 *pD
 u8 Efuse_CalculateWordCnts(u8 word_en)
 {
u8 word_cnts = 0;
+
if (!(word_en & BIT(0)))
word_cnts++; /*  0 : write enable */
if (!(word_en & BIT(1)))
diff --git a/drivers/staging/rtl8188eu/core/rtw_ieee80211.c 
b/drivers/staging/rtl8188eu/core/rtw_ieee80211.c
index 914c492..3e7a176 100644
--- a/drivers/staging/rtl8188eu/core/rtw_ieee80211.c
+++ b/drivers/staging/rtl8188eu/core/rtw_ieee80211.c
@@ -71,8 +71,8 @@ int rtw_get_bit_value_from_ieee_value(u8 val)
unsigned char dot11_rate_table[] = {
2, 4, 11, 22, 12, 18, 24, 36, 48,
72, 96, 108, 0}; /*  last element must be zero!! */
-
int i = 0;
+
while (dot11_rate_table[i] != 0) {
if (dot11_rate_table[i] == val)
return BIT(i);
@@ -162,6 +162,7 @@ u8 *rtw_get_ie(u8 *pbuf, int index, int *len, int limit)
 {
int tmp, i;
u8 *p;
+
if (limit < 1)
return NULL;
 
@@ -211,6 +212,7 @@ void rtw_set_supported_rate(u8 *SupportedRates, uint mode)
 uint   rtw_get_rateset_len(u8  *rateset)
 {
uint i = 0;
+
while (1) {
if ((rateset[i]) == 0)
break;
@@ -1002,6 +1004,7 @@ static int rtw_get_cipher_info(struct wlan_network 
*pnetwork)
unsigned char *pbuf;
int group_cipher = 0, pairwise_cipher = 0, is8021x = 0;
int ret = _FAIL;
+
pbuf = rtw_get_wpa_ie(>network.IEs[12], _ielen, 
pnetwork->network.IELength - 12);
 
if (pbuf && (wpa_ielen > 0)) {
diff --git a/drivers/staging/rtl8188eu/core/rtw_led.c 
b/drivers/staging/rtl8188eu/core/rtw_led.c
index c1478cf..8e3721c 100644
--- a/drivers/staging/rtl8188eu/core/rtw_led.c
+++ b/drivers/staging/rtl8188eu/core/rtw_led.c
@@ -40,6 +40,7 @@ void BlinkTimerCallback(unsigned long data)
 void BlinkWorkItemCallback(struct work_struct *work)
 {
struct LED_871x *pLed = container_of(work, struct LED_871x, 
BlinkWorkItem);
+
BlinkHandler(pLed);
 }
 
diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme.c 
b/drivers/staging/rtl8188eu/core/rtw_mlme.c
index 032f783..a719289 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme.c
@@ -659,6 +659,7 @@ void rtw_surveydone_event_callback(struct adapter   
*adapter, u8 *pbuf)
}
} else {
int s_ret;
+
set_fwstate(pmlmepriv, _FW_UNDER_LINKING);
pmlmepriv->to_join = false;