Re: [PATCH V4 2/3] mfd: Intel Platform Monitoring Technology support

[Mark D Rustad]

at 12:58 AM, Lee Jones  wrote:


If you do:

do {
int pos;

pos = pci_find_next_ext_capability(pdev, pos, 
PCI_EXT_CAP_ID_DVSEC);
if (!pos)
break;

Then you can invoke pci_find_next_ext_capability() once, no?


Part of your suggestion here won't work, because pos needs to be  
initialized to 0 the first time. As such it needs to be declared and  
initialized outside the loop. Other than that it may be ok.


--
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP

Re: Linux 4.4.174

[Mark D Rustad]

On Feb 9, 2019, at 12:13 AM, Greg KH  wrote:


On Fri, Feb 08, 2019 at 08:44:32PM -0800, Mark D Rustad wrote:

On Feb 8, 2019, at 2:54 AM, Greg KH  wrote:


diff --git a/Documentation/networking/ip-sysctl.txt
b/Documentation/networking/ip-sysctl.txt
index 2ea4c45cf1c8..7c229f59016f 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -112,14 +112,11 @@ min_adv_mss - INTEGER

 IP Fragmentation:

-ipfrag_high_thresh - INTEGER
-   Maximum memory used to reassemble IP fragments. When
-   ipfrag_high_thresh bytes of memory is allocated for this purpose,
-   the fragment handler will toss packets until ipfrag_low_thresh
-   is reached. This also serves as a maximum limit to namespaces
-   different from the initial one.
-
-ipfrag_low_thresh - INTEGER
+ipfrag_high_thresh - LONG INTEGER
+   Maximum memory used to reassemble IP fragments.
+
+ipfrag_low_thresh - LONG INTEGER
+   (Obsolete since linux-4.17)


It seems very strange to say that it is obsolete since 4.17 in a 4.4  
kernel.


4.17 is a point in time :)


Of course I understand, but some random non-kernel-developer tuning a  
kernel may be pretty puzzled. I don't know right off the top something  
brief that would be more generally meaningful, but maybe someone might.  
What does obsolete mean in this context? It exists but does nothing? It  
exists and does something but will eventually go away?


--
Mark Rustad, mrus...@gmail.com


signature.asc
Description: Message signed with OpenPGP

Re: Linux 4.4.174

[Mark D Rustad]

On Feb 8, 2019, at 2:54 AM, Greg KH  wrote:

diff --git a/Documentation/networking/ip-sysctl.txt  
b/Documentation/networking/ip-sysctl.txt

index 2ea4c45cf1c8..7c229f59016f 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -112,14 +112,11 @@ min_adv_mss - INTEGER

 IP Fragmentation:

-ipfrag_high_thresh - INTEGER
-   Maximum memory used to reassemble IP fragments. When
-   ipfrag_high_thresh bytes of memory is allocated for this purpose,
-   the fragment handler will toss packets until ipfrag_low_thresh
-   is reached. This also serves as a maximum limit to namespaces
-   different from the initial one.
-
-ipfrag_low_thresh - INTEGER
+ipfrag_high_thresh - LONG INTEGER
+   Maximum memory used to reassemble IP fragments.
+
+ipfrag_low_thresh - LONG INTEGER
+   (Obsolete since linux-4.17)


It seems very strange to say that it is obsolete since 4.17 in a 4.4 kernel.


Maximum memory used to reassemble IP fragments before the kernel
begins to remove incomplete fragment queues to free up resources.
The kernel still accepts new fragments for defragmentation.


--
Mark Rustad, mrus...@gmail.com


signature.asc
Description: Message signed with OpenPGP

Re: [virtio-dev] [RFC PATCH V2] virtio_pci: Add SR-IOV support

[Mark D Rustad]

> On Feb 25, 2018, at 7:20 AM, Yan Vugenfirer  wrote:
> 
> Small mistake in the commit message. Red Hat (Qumranet) vendor ID is 1af4, 
> virtio-net device ID is 1041.
> Should be:
> PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe
> VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe

You are so right. I will fix it before I send the updated patch that provides 
the generic helper that Christoph suggested. Thanks for catching my mislabeling.

--
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP

Re: [virtio-dev] [RFC PATCH V2] virtio_pci: Add SR-IOV support

[Mark D Rustad]

> On Feb 25, 2018, at 7:20 AM, Yan Vugenfirer  wrote:
> 
> Small mistake in the commit message. Red Hat (Qumranet) vendor ID is 1af4, 
> virtio-net device ID is 1041.
> Should be:
> PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe
> VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe

You are so right. I will fix it before I send the updated patch that provides 
the generic helper that Christoph suggested. Thanks for catching my mislabeling.

--
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 00/31 v2] PTI support for x86_32

[Mark D Rustad]

> On Feb 11, 2018, at 2:59 AM, Adam Borowski  wrote:
> 
>> Does Debian make it easy to upgrade to a 64-bit kernel if you have a
>> 32-bit install?
> 
> Quite easy, yeah.  Crossgrading userspace is not for the faint of the heart,
> but changing just the kernel is fine.

ISTR that iscsi doesn't work when running a 64-bit kernel with a 32-bit 
userspace. I remember someone offered kernel patches to fix it, but I think 
they were rejected. I haven't messed with that stuff in many years, so perhaps 
the userspace side now has accommodation for it. It might be something to check 
on.

--
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 00/31 v2] PTI support for x86_32

[Mark D Rustad]

> On Feb 11, 2018, at 2:59 AM, Adam Borowski  wrote:
> 
>> Does Debian make it easy to upgrade to a 64-bit kernel if you have a
>> 32-bit install?
> 
> Quite easy, yeah.  Crossgrading userspace is not for the faint of the heart,
> but changing just the kernel is fine.

ISTR that iscsi doesn't work when running a 64-bit kernel with a 32-bit 
userspace. I remember someone offered kernel patches to fix it, but I think 
they were rejected. I haven't messed with that stuff in many years, so perhaps 
the userspace side now has accommodation for it. It might be something to check 
on.

--
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 06/11] irqchip: mips-cpu: Drop unnecessary static

[Mark D Rustad]

> On Jul 15, 2017, at 1:59 PM, Julia Lawall <julia.law...@lip6.fr> wrote:
> 
> On Sat, 15 Jul 2017, Mark D Rustad wrote:
> 
>> 
>>> On Jul 15, 2017, at 1:07 PM, Julia Lawall <julia.law...@lip6.fr> wrote:
>>> 
>>> Drop static on a local variable, when the variable is initialized before
>>> any possible use.  Thus, the static has no benefit.
>> 
>> I think this is in error like the other one. I believe that the irq_chip 
>> structure needs a persistent lifetime.
> 
> I'm not following in this case.  chip is a pointer to a structure, not a
> structure.  Its address is not taken.  irq_set_chip_and_handler that uses
> it below is a fnuction, so it just gets the value.
> 
> julia

Sorry. My mistake. I didn't look quite hard enough at this one.

>> 
>>> The semantic patch that fixes this problem is as follows:
>>> (http://coccinelle.lip6.fr/)
>>> 
>>> // 
>>> @bad exists@
>>> position p;
>>> identifier x;
>>> type T;
>>> @@
>>> static T x@p;
>>> ...
>>> x = <+...x...+>
>>> 
>>> @@
>>> identifier x;
>>> expression e;
>>> type T;
>>> position p != bad.p;
>>> @@
>>> -static
>>> T x@p;
>>> ... when != x
>>>when strict
>>> ?x = e;
>>> // 
>>> 
>>> Signed-off-by: Julia Lawall <julia.law...@lip6.fr>
>>> 
>>> ---
>>> These patches are all independent of each other.
>>> 
>>> drivers/irqchip/irq-mips-cpu.c |2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff -u -p a/drivers/irqchip/irq-mips-cpu.c b/drivers/irqchip/irq-mips-cpu.c
>>> --- a/drivers/irqchip/irq-mips-cpu.c
>>> +++ b/drivers/irqchip/irq-mips-cpu.c
>>> @@ -154,7 +154,7 @@ asmlinkage void __weak plat_irq_dispatch
>>> static int mips_cpu_intc_map(struct irq_domain *d, unsigned int irq,
>>>  irq_hw_number_t hw)
>>> {
>>> -   static struct irq_chip *chip;
>>> +   struct irq_chip *chip;
>>> 
>>> if (hw < 2 && cpu_has_mipsmt) {
>>> /* Software interrupts are used for MT/CMT IPI */
>>> 
>> 
>> --
>> Mark Rustad, mrus...@gmail.com

--
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 06/11] irqchip: mips-cpu: Drop unnecessary static

[Mark D Rustad]

> On Jul 15, 2017, at 1:59 PM, Julia Lawall  wrote:
> 
> On Sat, 15 Jul 2017, Mark D Rustad wrote:
> 
>> 
>>> On Jul 15, 2017, at 1:07 PM, Julia Lawall  wrote:
>>> 
>>> Drop static on a local variable, when the variable is initialized before
>>> any possible use.  Thus, the static has no benefit.
>> 
>> I think this is in error like the other one. I believe that the irq_chip 
>> structure needs a persistent lifetime.
> 
> I'm not following in this case.  chip is a pointer to a structure, not a
> structure.  Its address is not taken.  irq_set_chip_and_handler that uses
> it below is a fnuction, so it just gets the value.
> 
> julia

Sorry. My mistake. I didn't look quite hard enough at this one.

>> 
>>> The semantic patch that fixes this problem is as follows:
>>> (http://coccinelle.lip6.fr/)
>>> 
>>> // 
>>> @bad exists@
>>> position p;
>>> identifier x;
>>> type T;
>>> @@
>>> static T x@p;
>>> ...
>>> x = <+...x...+>
>>> 
>>> @@
>>> identifier x;
>>> expression e;
>>> type T;
>>> position p != bad.p;
>>> @@
>>> -static
>>> T x@p;
>>> ... when != x
>>>when strict
>>> ?x = e;
>>> // 
>>> 
>>> Signed-off-by: Julia Lawall 
>>> 
>>> ---
>>> These patches are all independent of each other.
>>> 
>>> drivers/irqchip/irq-mips-cpu.c |2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff -u -p a/drivers/irqchip/irq-mips-cpu.c b/drivers/irqchip/irq-mips-cpu.c
>>> --- a/drivers/irqchip/irq-mips-cpu.c
>>> +++ b/drivers/irqchip/irq-mips-cpu.c
>>> @@ -154,7 +154,7 @@ asmlinkage void __weak plat_irq_dispatch
>>> static int mips_cpu_intc_map(struct irq_domain *d, unsigned int irq,
>>>  irq_hw_number_t hw)
>>> {
>>> -   static struct irq_chip *chip;
>>> +   struct irq_chip *chip;
>>> 
>>> if (hw < 2 && cpu_has_mipsmt) {
>>> /* Software interrupts are used for MT/CMT IPI */
>>> 
>> 
>> --
>> Mark Rustad, mrus...@gmail.com

--
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 06/11] irqchip: mips-cpu: Drop unnecessary static

[Mark D Rustad]

> On Jul 15, 2017, at 1:07 PM, Julia Lawall  wrote:
> 
> Drop static on a local variable, when the variable is initialized before
> any possible use.  Thus, the static has no benefit.

I think this is in error like the other one. I believe that the irq_chip 
structure needs a persistent lifetime.

> The semantic patch that fixes this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // 
> @bad exists@
> position p;
> identifier x;
> type T;
> @@
> static T x@p;
> ...
> x = <+...x...+>
> 
> @@
> identifier x;
> expression e;
> type T;
> position p != bad.p;
> @@
> -static
> T x@p;
> ... when != x
> when strict
> ?x = e;
> // 
> 
> Signed-off-by: Julia Lawall 
> 
> ---
> These patches are all independent of each other.
> 
> drivers/irqchip/irq-mips-cpu.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff -u -p a/drivers/irqchip/irq-mips-cpu.c b/drivers/irqchip/irq-mips-cpu.c
> --- a/drivers/irqchip/irq-mips-cpu.c
> +++ b/drivers/irqchip/irq-mips-cpu.c
> @@ -154,7 +154,7 @@ asmlinkage void __weak plat_irq_dispatch
> static int mips_cpu_intc_map(struct irq_domain *d, unsigned int irq,
>irq_hw_number_t hw)
> {
> - static struct irq_chip *chip;
> + struct irq_chip *chip;
> 
>   if (hw < 2 && cpu_has_mipsmt) {
>   /* Software interrupts are used for MT/CMT IPI */
> 

--
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 06/11] irqchip: mips-cpu: Drop unnecessary static

[Mark D Rustad]

> On Jul 15, 2017, at 1:07 PM, Julia Lawall  wrote:
> 
> Drop static on a local variable, when the variable is initialized before
> any possible use.  Thus, the static has no benefit.

I think this is in error like the other one. I believe that the irq_chip 
structure needs a persistent lifetime.

> The semantic patch that fixes this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // 
> @bad exists@
> position p;
> identifier x;
> type T;
> @@
> static T x@p;
> ...
> x = <+...x...+>
> 
> @@
> identifier x;
> expression e;
> type T;
> position p != bad.p;
> @@
> -static
> T x@p;
> ... when != x
> when strict
> ?x = e;
> // 
> 
> Signed-off-by: Julia Lawall 
> 
> ---
> These patches are all independent of each other.
> 
> drivers/irqchip/irq-mips-cpu.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff -u -p a/drivers/irqchip/irq-mips-cpu.c b/drivers/irqchip/irq-mips-cpu.c
> --- a/drivers/irqchip/irq-mips-cpu.c
> +++ b/drivers/irqchip/irq-mips-cpu.c
> @@ -154,7 +154,7 @@ asmlinkage void __weak plat_irq_dispatch
> static int mips_cpu_intc_map(struct irq_domain *d, unsigned int irq,
>irq_hw_number_t hw)
> {
> - static struct irq_chip *chip;
> + struct irq_chip *chip;
> 
>   if (hw < 2 && cpu_has_mipsmt) {
>   /* Software interrupts are used for MT/CMT IPI */
> 

--
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 07/11] mfd: Drop unnecessary static

[Mark D Rustad]

> On Jul 15, 2017, at 1:07 PM, Julia Lawall  wrote:
> 
> Drop static on a local variable, when the variable is initialized before
> any possible use.  Thus, the static has no benefit.

I think in this case the use relies on the structure continuing to exist, so a 
stack object is not an acceptable substitute. Just because it is initialized 
doesn't mean that it doesn't need a persistent lifetime.

> The semantic patch that fixes this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // 
> @bad exists@
> position p;
> identifier x;
> type T;
> @@
> static T x@p;
> ...
> x = <+...x...+>
> 
> @@
> identifier x;
> expression e;
> type T;
> position p != bad.p;
> @@
> -static
> T x@p;
> ... when != x
> when strict
> ?x = e;
> // 
> 
> Signed-off-by: Julia Lawall 
> 
> ---
> These patches are all independent of each other.
> 
> drivers/mfd/twl4030-irq.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff -u -p a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
> --- a/drivers/mfd/twl4030-irq.c
> +++ b/drivers/mfd/twl4030-irq.c
> @@ -685,7 +685,7 @@ int twl4030_sih_setup(struct device *dev
> 
> int twl4030_init_irq(struct device *dev, int irq_num)
> {
> - static struct irq_chip  twl4030_irq_chip;
> + struct irq_chip twl4030_irq_chip;
>   int status, i;
>   int irq_base, irq_end, nr_irqs;
>   struct  device_node *node = dev->of_node;

--
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 07/11] mfd: Drop unnecessary static

[Mark D Rustad]

> On Jul 15, 2017, at 1:07 PM, Julia Lawall  wrote:
> 
> Drop static on a local variable, when the variable is initialized before
> any possible use.  Thus, the static has no benefit.

I think in this case the use relies on the structure continuing to exist, so a 
stack object is not an acceptable substitute. Just because it is initialized 
doesn't mean that it doesn't need a persistent lifetime.

> The semantic patch that fixes this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // 
> @bad exists@
> position p;
> identifier x;
> type T;
> @@
> static T x@p;
> ...
> x = <+...x...+>
> 
> @@
> identifier x;
> expression e;
> type T;
> position p != bad.p;
> @@
> -static
> T x@p;
> ... when != x
> when strict
> ?x = e;
> // 
> 
> Signed-off-by: Julia Lawall 
> 
> ---
> These patches are all independent of each other.
> 
> drivers/mfd/twl4030-irq.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff -u -p a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
> --- a/drivers/mfd/twl4030-irq.c
> +++ b/drivers/mfd/twl4030-irq.c
> @@ -685,7 +685,7 @@ int twl4030_sih_setup(struct device *dev
> 
> int twl4030_init_irq(struct device *dev, int irq_num)
> {
> - static struct irq_chip  twl4030_irq_chip;
> + struct irq_chip twl4030_irq_chip;
>   int status, i;
>   int irq_base, irq_end, nr_irqs;
>   struct  device_node *node = dev->of_node;

--
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP

Re: MBR partitions slow?

[Mark D Rustad]

Ulrich Windl  wrote:


So without partition the throughput is about twice as high! Why?


My first thought is that by starting at block 0 the accesses were aligned  
with the flash block size of the device. By starting at a partition, the  
accesses probably were not so aligned.


--
Mark Rustad, mrus...@gmail.com


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: MBR partitions slow?

[Mark D Rustad]

Ulrich Windl  wrote:


So without partition the throughput is about twice as high! Why?


My first thought is that by starting at block 0 the accesses were aligned  
with the flash block size of the device. By starting at a partition, the  
accesses probably were not so aligned.


--
Mark Rustad, mrus...@gmail.com


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] CodingStyle: Clarify and complete chapter 7

[Mark D Rustad]

Jonathan Corbet  wrote:


On Mon, 25 Jul 2016 14:29:06 +0200
Jean Delvare  wrote:


Chapter 7 (Centralized exiting of functions) of the coding style
documentation is unclear at times, and lacks some information (such
as the possibility to indent labels with a single space.) Clarify and
complete it.


OK, I've applied this (finally) to the docs tree, sorry for sitting on it
for so long.  One question, though...

A common type of bug to be aware of is "one err bugs" which look like  
this:


-   err:
+err:
kfree(foo->bar);
kfree(foo);
return ret;

 The bug in this code is that on some exit paths "foo" is NULL.  Normally the


...except that kfree() can handle null pointers just fine, so this isn't
actually a bug, right?  Someday when somebody has time it would be good to
come up with a better example.


But if foo is NULL, foo->bar is not NULL and so kfree will have a problem  
with it. So this is a bug.


--
Mark Rustad, mrus...@gmail.com


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] CodingStyle: Clarify and complete chapter 7

[Mark D Rustad]

Jonathan Corbet  wrote:


On Mon, 25 Jul 2016 14:29:06 +0200
Jean Delvare  wrote:


Chapter 7 (Centralized exiting of functions) of the coding style
documentation is unclear at times, and lacks some information (such
as the possibility to indent labels with a single space.) Clarify and
complete it.


OK, I've applied this (finally) to the docs tree, sorry for sitting on it
for so long.  One question, though...

A common type of bug to be aware of is "one err bugs" which look like  
this:


-   err:
+err:
kfree(foo->bar);
kfree(foo);
return ret;

 The bug in this code is that on some exit paths "foo" is NULL.  Normally the


...except that kfree() can handle null pointers just fine, so this isn't
actually a bug, right?  Someday when somebody has time it would be good to
come up with a better example.


But if foo is NULL, foo->bar is not NULL and so kfree will have a problem  
with it. So this is a bug.


--
Mark Rustad, mrus...@gmail.com


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero

[Mark D Rustad]

Jarod Wilson  wrote:


On Fri, May 06, 2016 at 11:43:17PM +, Rustad, Mark D wrote:

Denys Vlasenko  wrote:


Users report that under VMWare, er32(TIMINCA) returns zero.
This causes division by zero at init time as follows:

==>incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
   for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
   /* latch SYSTIMH on read of SYSTIML */
   systim_next = (cycle_t)er32(SYSTIML);
   systim_next |= (cycle_t)er32(SYSTIMH) << 32;

   time_delta = systim_next - systim;
   temp = time_delta;
>  rem = do_div(temp, incvalue);

This change makes kernel survive this, and users report that
NIC does work after this change.

Since on real hardware incvalue is never zero, this should not affect
real hardware use case.

...
I seem to recall that this was rejected before because it really is  
VMWare's

bug and, if they fix it, any existing VMs that use this will just work.
Changing the driver will only fix it for vms that install a new driver. I
don't object to doing it, it just seems like not the most effective  
place to

address the issue.


You could also have people who never update VMWare, for whom a kernel
work-around would be better. I think it'd be best to address it both at
the driver level and the emulated hardware level, to improve things for
the most possible users. Those who update neither hypervisor or
kernel/driver, well, they reap what they sow.


That is a sound argument for doing both. I would expect that there are more  
frozen VM images than host environments, but I can certainly imagine that  
some choose to freeze their host. Of course if everything is frozen there  
is no point at all. :-)


I am on an extended vacation, and don't work on e1000e anyway, so I will  
quit my kibitzing here.


--
Mark Rustad, mrus...@gmail.com


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero

[Mark D Rustad]

Jarod Wilson  wrote:


On Fri, May 06, 2016 at 11:43:17PM +, Rustad, Mark D wrote:

Denys Vlasenko  wrote:


Users report that under VMWare, er32(TIMINCA) returns zero.
This causes division by zero at init time as follows:

==>incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
   for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
   /* latch SYSTIMH on read of SYSTIML */
   systim_next = (cycle_t)er32(SYSTIML);
   systim_next |= (cycle_t)er32(SYSTIMH) << 32;

   time_delta = systim_next - systim;
   temp = time_delta;
>  rem = do_div(temp, incvalue);

This change makes kernel survive this, and users report that
NIC does work after this change.

Since on real hardware incvalue is never zero, this should not affect
real hardware use case.

...
I seem to recall that this was rejected before because it really is  
VMWare's

bug and, if they fix it, any existing VMs that use this will just work.
Changing the driver will only fix it for vms that install a new driver. I
don't object to doing it, it just seems like not the most effective  
place to

address the issue.


You could also have people who never update VMWare, for whom a kernel
work-around would be better. I think it'd be best to address it both at
the driver level and the emulated hardware level, to improve things for
the most possible users. Those who update neither hypervisor or
kernel/driver, well, they reap what they sow.


That is a sound argument for doing both. I would expect that there are more  
frozen VM images than host environments, but I can certainly imagine that  
some choose to freeze their host. Of course if everything is frozen there  
is no point at all. :-)


I am on an extended vacation, and don't work on e1000e anyway, so I will  
quit my kibitzing here.


--
Mark Rustad, mrus...@gmail.com


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: Determination for the number of named function parameters (with SmPL)

[Mark D Rustad]

On Dec 1, 2014, at 3:12 AM, SF Markus Elfring  
wrote:

> Hello,
> 
> Would you like to know how many named function parameters are used in the 
> source files?
> 
> How do you think about to try the following semantic query approach out a bit 
> more?
> 
> @initialize:python@
> @@
> import sys
> import sqlite3 as SQLite
> connection = SQLite.connect(":memory:")
> c = connection.cursor()
> c.execute("""create table numbers (number integer)""")
> delimiter = "|"
> 
> def store_number(count):
>"""Add an integer to an internal list."""
>c.execute("""insert into numbers (number) values (?)""",
>  (count, )
> )
> 
> @counting_parameters@
> identifier work;
> parameter list[number] pl;
> type return_type;
> @@
> return_type work(pl)
> {
>  ...
> }
> 
> @script:python collection@
> count << counting_parameters.number;
> @@
> store_number(count)
> 
> @finalize:python@
> @@
> c.execute("""select count(*) nr from numbers""")
> result = c.fetchone()
> 
> if result[0] > 0:
>   c.execute("""create index x on numbers (number)""")
>   c.execute("select number, count(*) nr from numbers group by number")
>   sys.stdout.write(delimiter.join( ("number", "counter") ))
>   sys.stdout.write("\r\n")
>   for result in c:
>  sys.stdout.write(delimiter.join((str(result[0]),
>   str(result[1])
>  )))
>  sys.stdout.write("\r\n")
> else:
>   sys.stderr.write("No result for this analysis!\n")
> 
> connection.close()
> 
> 
> 
> elfring@Sonne:~/Projekte/Coccinelle/Probe> XX=$(date) && spatch.opt -timeout 
> 12 -sp-file list_parameter_numbers1.cocci -dir /usr/src/linux-stable > 
> list_parameter_numbers1.txt 2> list_parameter_numbers1-errors.txt ; 
> YY=$(date) && echo "$XX * $YY"
> ...
> elfring@Sonne:~/Projekte/Coccinelle/Probe> cat list_parameter_numbers1.txt
> number|counter
> 0|29

I think the results are dubious. Only 29 functions with no parameters? That 
can't be right.

> 1|18261
> 2|15374
> 3|12237
> 4|8159
> 5|4339
> 6|2701
> 7|1183
> 8|518
> 9|260
> 10|146
> 11|83
> 12|42
> 13|21
> 14|9
> 15|7
> 16|2
> 17|4
> 18|1
> 21|1
> 22|1
> 
> 
> Do you find such an analysis result from the source files for Linux 3.17.4
> interesting for further considerations?
> 
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: Determination for the number of named function parameters (with SmPL)

[Mark D Rustad]

On Dec 1, 2014, at 3:12 AM, SF Markus Elfring elfr...@users.sourceforge.net 
wrote:

 Hello,
 
 Would you like to know how many named function parameters are used in the 
 source files?
 
 How do you think about to try the following semantic query approach out a bit 
 more?
 
 @initialize:python@
 @@
 import sys
 import sqlite3 as SQLite
 connection = SQLite.connect(:memory:)
 c = connection.cursor()
 c.execute(create table numbers (number integer))
 delimiter = |
 
 def store_number(count):
Add an integer to an internal list.
c.execute(insert into numbers (number) values (?),
  (count, )
 )
 
 @counting_parameters@
 identifier work;
 parameter list[number] pl;
 type return_type;
 @@
 return_type work(pl)
 {
  ...
 }
 
 @script:python collection@
 count  counting_parameters.number;
 @@
 store_number(count)
 
 @finalize:python@
 @@
 c.execute(select count(*) nr from numbers)
 result = c.fetchone()
 
 if result[0]  0:
   c.execute(create index x on numbers (number))
   c.execute(select number, count(*) nr from numbers group by number)
   sys.stdout.write(delimiter.join( (number, counter) ))
   sys.stdout.write(\r\n)
   for result in c:
  sys.stdout.write(delimiter.join((str(result[0]),
   str(result[1])
  )))
  sys.stdout.write(\r\n)
 else:
   sys.stderr.write(No result for this analysis!\n)
 
 connection.close()
 
 
 
 elfring@Sonne:~/Projekte/Coccinelle/Probe XX=$(date)  spatch.opt -timeout 
 12 -sp-file list_parameter_numbers1.cocci -dir /usr/src/linux-stable  
 list_parameter_numbers1.txt 2 list_parameter_numbers1-errors.txt ; 
 YY=$(date)  echo $XX * $YY
 ...
 elfring@Sonne:~/Projekte/Coccinelle/Probe cat list_parameter_numbers1.txt
 number|counter
 0|29

I think the results are dubious. Only 29 functions with no parameters? That 
can't be right.

 1|18261
 2|15374
 3|12237
 4|8159
 5|4339
 6|2701
 7|1183
 8|518
 9|260
 10|146
 11|83
 12|42
 13|21
 14|9
 15|7
 16|2
 17|4
 18|1
 21|1
 22|1
 
 
 Do you find such an analysis result from the source files for Linux 3.17.4
 interesting for further considerations?
 
 Regards,
 Markus
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] char: hw_random: core.c: Changed from using strncpy to strlcpy

[Mark D Rustad]

On Oct 11, 2014, at 3:36 PM, Rickard Strandqvist 
 wrote:

> Changed from using strncpy to strlcpy to simplify the code

Actually you changed from using strncat to strlcat.

> Signed-off-by: Rickard Strandqvist 
> ---
> drivers/char/hw_random/core.c |   12 
> 1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index aa30a25..1500cfd 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -281,7 +281,6 @@ static ssize_t hwrng_attr_available_show(struct device 
> *dev,
>char *buf)
> {
>   int err;
> - ssize_t ret = 0;
>   struct hwrng *rng;
> 
>   err = mutex_lock_interruptible(_mutex);
> @@ -289,16 +288,13 @@ static ssize_t hwrng_attr_available_show(struct device 
> *dev,
>   return -ERESTARTSYS;
>   buf[0] = '\0';
>   list_for_each_entry(rng, _list, list) {
> - strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> - ret += strlen(rng->name);
> - strncat(buf, " ", PAGE_SIZE - ret - 1);
> - ret++;
> + strlcat(buf, rng->name, PAGE_SIZE);
> + strlcat(buf, " ", PAGE_SIZE);
>   }
> - strncat(buf, "\n", PAGE_SIZE - ret - 1);
> - ret++;
> + strlcat(buf, "\n", PAGE_SIZE);
>   mutex_unlock(_mutex);
> 
> - return ret;
> + return strlen(buf);
> }
> 
> static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
> -- 
> 1.7.10.4

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] char: hw_random: core.c: Changed from using strncpy to strlcpy

[Mark D Rustad]

On Oct 11, 2014, at 3:36 PM, Rickard Strandqvist 
rickard_strandqv...@spectrumdigital.se wrote:

 Changed from using strncpy to strlcpy to simplify the code

Actually you changed from using strncat to strlcat.

 Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
 ---
 drivers/char/hw_random/core.c |   12 
 1 file changed, 4 insertions(+), 8 deletions(-)
 
 diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
 index aa30a25..1500cfd 100644
 --- a/drivers/char/hw_random/core.c
 +++ b/drivers/char/hw_random/core.c
 @@ -281,7 +281,6 @@ static ssize_t hwrng_attr_available_show(struct device 
 *dev,
char *buf)
 {
   int err;
 - ssize_t ret = 0;
   struct hwrng *rng;
 
   err = mutex_lock_interruptible(rng_mutex);
 @@ -289,16 +288,13 @@ static ssize_t hwrng_attr_available_show(struct device 
 *dev,
   return -ERESTARTSYS;
   buf[0] = '\0';
   list_for_each_entry(rng, rng_list, list) {
 - strncat(buf, rng-name, PAGE_SIZE - ret - 1);
 - ret += strlen(rng-name);
 - strncat(buf,  , PAGE_SIZE - ret - 1);
 - ret++;
 + strlcat(buf, rng-name, PAGE_SIZE);
 + strlcat(buf,  , PAGE_SIZE);
   }
 - strncat(buf, \n, PAGE_SIZE - ret - 1);
 - ret++;
 + strlcat(buf, \n, PAGE_SIZE);
   mutex_unlock(rng_mutex);
 
 - return ret;
 + return strlen(buf);
 }
 
 static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
 -- 
 1.7.10.4

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

[PATCH] sched: Remove nested extern

[Mark D Rustad]

Avoid W=2 nested-externs warning by moving the nested extern to
a normal extern. This eliminates that warning which is generated
for every inclusion of sched.h in a kernel build when W=2 is used.
This also removes a point of maintenance if the definition of
delayacct_on were ever to change.

Signed-off-by: Mark Rustad 
---
 include/linux/delayacct.h |1 -
 include/linux/sched.h |3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/delayacct.h b/include/linux/delayacct.h
index 6cee17c22313..51229790af00 100644
--- a/include/linux/delayacct.h
+++ b/include/linux/delayacct.h
@@ -30,7 +30,6 @@
 
 #ifdef CONFIG_TASK_DELAY_ACCT
 
-extern int delayacct_on;   /* Delay accounting turned on/off */
 extern struct kmem_cache *delayacct_cache;
 extern void delayacct_init(void);
 extern void __delayacct_tsk_init(struct task_struct *);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 5c2c885ee52b..1f1dcfdcd92c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -825,6 +825,8 @@ struct task_delay_info {
u64 freepages_delay;/* wait for memory reclaim */
u32 freepages_count;/* total count of memory reclaim */
 };
+
+extern int delayacct_on;   /* Delay accounting turned on/off */
 #endif /* CONFIG_TASK_DELAY_ACCT */
 
 static inline int sched_info_on(void)
@@ -832,7 +834,6 @@ static inline int sched_info_on(void)
 #ifdef CONFIG_SCHEDSTATS
return 1;
 #elif defined(CONFIG_TASK_DELAY_ACCT)
-   extern int delayacct_on;
return delayacct_on;
 #else
return 0;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] sched: Remove nested extern

[Mark D Rustad]

Avoid W=2 nested-externs warning by moving the nested extern to
a normal extern. This eliminates that warning which is generated
for every inclusion of sched.h in a kernel build when W=2 is used.
This also removes a point of maintenance if the definition of
delayacct_on were ever to change.

Signed-off-by: Mark Rustad mark.d.rus...@intel.com
---
 include/linux/delayacct.h |1 -
 include/linux/sched.h |3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/delayacct.h b/include/linux/delayacct.h
index 6cee17c22313..51229790af00 100644
--- a/include/linux/delayacct.h
+++ b/include/linux/delayacct.h
@@ -30,7 +30,6 @@
 
 #ifdef CONFIG_TASK_DELAY_ACCT
 
-extern int delayacct_on;   /* Delay accounting turned on/off */
 extern struct kmem_cache *delayacct_cache;
 extern void delayacct_init(void);
 extern void __delayacct_tsk_init(struct task_struct *);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 5c2c885ee52b..1f1dcfdcd92c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -825,6 +825,8 @@ struct task_delay_info {
u64 freepages_delay;/* wait for memory reclaim */
u32 freepages_count;/* total count of memory reclaim */
 };
+
+extern int delayacct_on;   /* Delay accounting turned on/off */
 #endif /* CONFIG_TASK_DELAY_ACCT */
 
 static inline int sched_info_on(void)
@@ -832,7 +834,6 @@ static inline int sched_info_on(void)
 #ifdef CONFIG_SCHEDSTATS
return 1;
 #elif defined(CONFIG_TASK_DELAY_ACCT)
-   extern int delayacct_on;
return delayacct_on;
 #else
return 0;

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Overriding -Werror

[Mark D Rustad]

On Aug 15, 2014, at 9:34 PM, Brian Norris  wrote:

> Hi Mark,
> 
> (BTW, your mailer is creating some pretty long, unwrapped lines. I've
> rewrapped them when quoting below.)

Sorry about that. I'll try to remember to deal with it on my end.

> On Fri, Aug 15, 2014 at 08:36:07PM -0700, Mark D Rustad wrote:
>> On Aug 15, 2014, at 12:33 PM, Brian Norris  
>> wrote:
>>> On Fri, Aug 15, 2014 at 02:30:49AM -0700, Jeff Kirsher wrote:
>>>> Funny that you bring this up because I have ~60 patches in my queue to
>>>> resolve several thousand of these warnings.  Half of the patches
>>>> actually resolve warnings that can be resolved and the other half
>>>> implement compiler diagnostic control macros to help silence warnings.
>>>> All these were the work of a co-worker Mark Rustad, below is the patch
>>>> he put together to implement the compiler diagnostic control macros.
>>> 
>>> While fixing warnings is usually a good thing (at least when done right;
>>> there are plenty of ways to fight with the compiler over silly things,
>>> but that's another discussion),
>> 
>> I have said at some presentations on the subject that resolving
>> warnings is not something you want an intern to do.
> 
> Nice.
> 
>>> I think that my issue is still
>>> orthogonal to the one you're addressing. In my estimation, it is
>>> impossible to guarantee that the entire kernel (including drivers) will
>>> build without any warnings, across all levels of warning verbosity.
>>> Thus, even with a valiant effort to fix or annotate all warnings, we
>>> still won't get to the point where I can build 'make ARCH=mips W=1', if
>>> -Werror is active.
>> 
>> Actually, some years ago, I got a MIPS Linux kernel to compile clean
>> with even more warnings than W=12 provides.
> 
> Congrats!
> 
>> It can be done, but it certainly is not a state that is required and
>> cannot be maintained across all configurations, architectures and
>> compiler versions. This is the real world.
> 
> Right, and this is the crux of why I would like to have the option of
> disabling -Werror systematically.
> 
>>> Besides, when testing *new* code, it's even more likely to have new
>>> warnings, and I'd like to see as many as possible, without -Werror
>>> getting in the way.
>> 
>> I have to say that I rather like -Werror.
> 
> It has its uses. I think it can be a pretty good option whenever the
> compiler's warning level is kept to a reasonable level.
> 
> -Wextra, for instance, enables a lot of warnings that can be problematic
> for little practical benefit (like -Wsign-compare, which notably is
> explicitly overridden for x86).

Hmm. I wasn't aware that x86 had that disabled. Sign-compare actually does
find some kinds of bugs. Those warnings are worth resolving for that reason.
Being explicit about whether you want the signed or unsigned comparison is
a good thing.

>> One thing that not a lot of people are aware of is that you can
>> selectively allow some warnings. -Wno-error=shadow would allow shadow
>> warnings to be reported without being treated as errors.
> 
> That's interesting. I glazed over this option because I misinterpreted
> the second sentence of this note in the gcc manpage:
> 
>  "Note that specifying -Werror=foo automatically implies -Wfoo.
>  However, -Wno-error=foo does not imply anything."
> 
> [ Really, -Wno-error=foo doesn't imply anything? So why does it exist? ;) ]

Yeah, it means that it doesn't imply anything about whether the warning
is enabled or not. It will neither enable nor disable the warning as a
side-effect, unlike -Werror= which will enable it. So you can add -Wno
error=... without regard to whether the given warning was enabled or
not. It kind of makes sense, but is not explained very well.

> But now that you mention it, I think it could work as a hack for my
> builds right now, since it isn't overridden by a blanket -Werror found
> later on the command linie. So if I do
> 
>  make ARCH=mips W=1 KCFLAGS="-Wno-error=...(long list of warnings that break 
> the build)..."

And still be able to see them. Not bad.

> then I can systematically perform the build-tests I'd like.

Yes.

>>> So I still think -Werror is fundamentally wrong in some cases, and I
>>> would like to pursue some approach like in my original post.
>>> 
>>> BTW, for a little more context: I realize the output of 'make W=[123]'
>>> may not be very useful on its own, sometimes, but it's actually pretty
>>> useful to quickly catch potential issues in new code, by diff'ing the
>>> warnin

Re: Overriding -Werror

[Mark D Rustad]

On Aug 15, 2014, at 9:34 PM, Brian Norris computersforpe...@gmail.com wrote:

 Hi Mark,
 
 (BTW, your mailer is creating some pretty long, unwrapped lines. I've
 rewrapped them when quoting below.)

Sorry about that. I'll try to remember to deal with it on my end.

 On Fri, Aug 15, 2014 at 08:36:07PM -0700, Mark D Rustad wrote:
 On Aug 15, 2014, at 12:33 PM, Brian Norris computersforpe...@gmail.com 
 wrote:
 On Fri, Aug 15, 2014 at 02:30:49AM -0700, Jeff Kirsher wrote:
 Funny that you bring this up because I have ~60 patches in my queue to
 resolve several thousand of these warnings.  Half of the patches
 actually resolve warnings that can be resolved and the other half
 implement compiler diagnostic control macros to help silence warnings.
 All these were the work of a co-worker Mark Rustad, below is the patch
 he put together to implement the compiler diagnostic control macros.
 
 While fixing warnings is usually a good thing (at least when done right;
 there are plenty of ways to fight with the compiler over silly things,
 but that's another discussion),
 
 I have said at some presentations on the subject that resolving
 warnings is not something you want an intern to do.
 
 Nice.
 
 I think that my issue is still
 orthogonal to the one you're addressing. In my estimation, it is
 impossible to guarantee that the entire kernel (including drivers) will
 build without any warnings, across all levels of warning verbosity.
 Thus, even with a valiant effort to fix or annotate all warnings, we
 still won't get to the point where I can build 'make ARCH=mips W=1', if
 -Werror is active.
 
 Actually, some years ago, I got a MIPS Linux kernel to compile clean
 with even more warnings than W=12 provides.
 
 Congrats!
 
 It can be done, but it certainly is not a state that is required and
 cannot be maintained across all configurations, architectures and
 compiler versions. This is the real world.
 
 Right, and this is the crux of why I would like to have the option of
 disabling -Werror systematically.
 
 Besides, when testing *new* code, it's even more likely to have new
 warnings, and I'd like to see as many as possible, without -Werror
 getting in the way.
 
 I have to say that I rather like -Werror.
 
 It has its uses. I think it can be a pretty good option whenever the
 compiler's warning level is kept to a reasonable level.
 
 -Wextra, for instance, enables a lot of warnings that can be problematic
 for little practical benefit (like -Wsign-compare, which notably is
 explicitly overridden for x86).

Hmm. I wasn't aware that x86 had that disabled. Sign-compare actually does
find some kinds of bugs. Those warnings are worth resolving for that reason.
Being explicit about whether you want the signed or unsigned comparison is
a good thing.

 One thing that not a lot of people are aware of is that you can
 selectively allow some warnings. -Wno-error=shadow would allow shadow
 warnings to be reported without being treated as errors.
 
 That's interesting. I glazed over this option because I misinterpreted
 the second sentence of this note in the gcc manpage:
 
  Note that specifying -Werror=foo automatically implies -Wfoo.
  However, -Wno-error=foo does not imply anything.
 
 [ Really, -Wno-error=foo doesn't imply anything? So why does it exist? ;) ]

Yeah, it means that it doesn't imply anything about whether the warning
is enabled or not. It will neither enable nor disable the warning as a
side-effect, unlike -Werror= which will enable it. So you can add -Wno
error=... without regard to whether the given warning was enabled or
not. It kind of makes sense, but is not explained very well.

 But now that you mention it, I think it could work as a hack for my
 builds right now, since it isn't overridden by a blanket -Werror found
 later on the command linie. So if I do
 
  make ARCH=mips W=1 KCFLAGS=-Wno-error=...(long list of warnings that break 
 the build)...

And still be able to see them. Not bad.

 then I can systematically perform the build-tests I'd like.

Yes.

 So I still think -Werror is fundamentally wrong in some cases, and I
 would like to pursue some approach like in my original post.
 
 BTW, for a little more context: I realize the output of 'make W=[123]'
 may not be very useful on its own, sometimes, but it's actually pretty
 useful to quickly catch potential issues in new code, by diff'ing the
 warnings in the before/after build logs. In this case, it's not helpful
 at all if the first build fails due to dubious warnings. I'm doing
 this in the context of Aiaiai [1]. Right now, I have to keep around a
 few local patches to remove -Werror from arch/{mips,sh}.
 
 The problem is that when a kernel build throws over 125,000 warnings,
 it just becomes completely useless. That was what kind of set me off.
 
 Yeah that's bad. But it *still* can provide a helpful diff for tools
 like Aiaiai. The difference between 125,000 and 125,001 warnings still
 can be determined automatically for new code, although

Re: Overriding -Werror

[Mark D Rustad]

Brian,

On Aug 15, 2014, at 12:33 PM, Brian Norris  wrote:

> Hi,
> 
> On Fri, Aug 15, 2014 at 02:30:49AM -0700, Jeff Kirsher wrote:
>> Funny that you bring this up because I have ~60 patches in my queue to
>> resolve several thousand of these warnings.  Half of the patches
>> actually resolve warnings that can be resolved and the other half
>> implement compiler diagnostic control macros to help silence warnings.
>> All these were the work of a co-worker Mark Rustad, below is the patch
>> he put together to implement the compiler diagnostic control macros.
> 
> While fixing warnings is usually a good thing (at least when done right;
> there are plenty of ways to fight with the compiler over silly things,
> but that's another discussion),

I have said at some presentations on the subject that resolving warnings is not 
something you want an intern to do.

> I think that my issue is still
> orthogonal to the one you're addressing. In my estimation, it is
> impossible to guarantee that the entire kernel (including drivers) will
> build without any warnings, across all levels of warning verbosity.
> Thus, even with a valiant effort to fix or annotate all warnings, we
> still won't get to the point where I can build 'make ARCH=mips W=1', if
> -Werror is active.

Actually, some years ago, I got a MIPS Linux kernel to compile clean with even 
more warnings than W=12 provides. It can be done, but it certainly is not a 
state that is required and cannot be maintained across all configurations, 
architectures and compiler versions. This is the real world.

> Besides, when testing *new* code, it's even more likely to have new
> warnings, and I'd like to see as many as possible, without -Werror
> getting in the way.

I have to say that I rather like -Werror. One thing that not a lot of people 
are aware of is that you can selectively allow some warnings. -Wno-error=shadow 
would allow shadow warnings to be reported without being treated as errors.

> So I still think -Werror is fundamentally wrong in some cases, and I
> would like to pursue some approach like in my original post.
> 
> BTW, for a little more context: I realize the output of 'make W=[123]'
> may not be very useful on its own, sometimes, but it's actually pretty
> useful to quickly catch potential issues in new code, by diff'ing the
> warnings in the before/after build logs. In this case, it's not helpful
> at all if the first build "fails" due to dubious warnings. I'm doing
> this in the context of Aiaiai [1]. Right now, I have to keep around a
> few local patches to remove -Werror from arch/{mips,sh}.

The problem is that when a kernel build throws over 125,000 warnings, it just 
becomes completely useless. That was what kind of set me off. I did wind up 
pushing this rock further up the hill than I really meant to. Still, I got the 
build under 1,400 warnings, and I now know how to address most of them in a 
systematic way.

>> commit 7b9aace02b2405f0714bc08c424b72e6962f1c2e
>> Author: Mark Rustad 
>> Date:   Fri Aug 15 01:43:44 2014 -0700
>> 
>>compiler: Add diagnostic control macros
>> 
>>Add macros to control diagnostic messages where needed. These
>>are used to silence warning messages that are expected, normal
>>and do not indicate any sort of problem. Reducing the stream
>>of messages in this way helps possible problems to stand out.
>> 
>>The macros provided are:
>>DIAG_PUSH() - to save diagnostic settings
>>DIAG_POP() - to restore diagnostic settings
>>DIAG_IGNORE(option) - to ignore a particular warning
>>DIAG_GCC_IGNORE(option) - DIAG_IGNORE for gcc only
>>DIAG_CLANG_IGNORE(option) - DIAG_IGNORE for clang only
>> 
>>Signed-off-by: Mark Rustad 
>> 
>> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
>> index d1e49d5..039b112 100644
>> --- a/include/linux/compiler-clang.h
>> +++ b/include/linux/compiler-clang.h
>> @@ -10,3 +10,29 @@
>> #undef uninitialized_var
>> #define uninitialized_var(x) x = *(&(x))
>> #endif
>> +
>> +/*
>> + * Provide macros to manipulate diagnostic messages when possible.
>> + * DIAG_PUSH pushes the diagnostic settings
>> + * DIAG_POP pops the diagnostic settings
>> + * DIAG_IGNORE(x) changes the given diagnostic setting to ignore
>> + *
>> + * Example:
>> + *DIAG_PUSH DIAG_IGNORE(aggregate-return)
>> + *struct timespec ns_to_timespec(const s64 nsec)
>> + *{
>> + *...
>> + *}
>> + *DIAG_POP
>> + *
>> + * Will prevent the warning on compilation of the function. Other
>> + * steps are necessary to do the same thing for the call sites.
>> + */
> 
> While I do not want to disparage your/Mark's work here, my first thought
> about this kind of annotation is that it seems to be a pretty big burden
> to have to annotate all code with these sorts of things.

I wouldn't suggest annotating everything. However note that the annotations can 
serve as a notice that something has been analyzed and deemed ok. 

Re: Overriding -Werror

[Mark D Rustad]

Brian,

On Aug 15, 2014, at 12:33 PM, Brian Norris computersforpe...@gmail.com wrote:

 Hi,
 
 On Fri, Aug 15, 2014 at 02:30:49AM -0700, Jeff Kirsher wrote:
 Funny that you bring this up because I have ~60 patches in my queue to
 resolve several thousand of these warnings.  Half of the patches
 actually resolve warnings that can be resolved and the other half
 implement compiler diagnostic control macros to help silence warnings.
 All these were the work of a co-worker Mark Rustad, below is the patch
 he put together to implement the compiler diagnostic control macros.
 
 While fixing warnings is usually a good thing (at least when done right;
 there are plenty of ways to fight with the compiler over silly things,
 but that's another discussion),

I have said at some presentations on the subject that resolving warnings is not 
something you want an intern to do.

 I think that my issue is still
 orthogonal to the one you're addressing. In my estimation, it is
 impossible to guarantee that the entire kernel (including drivers) will
 build without any warnings, across all levels of warning verbosity.
 Thus, even with a valiant effort to fix or annotate all warnings, we
 still won't get to the point where I can build 'make ARCH=mips W=1', if
 -Werror is active.

Actually, some years ago, I got a MIPS Linux kernel to compile clean with even 
more warnings than W=12 provides. It can be done, but it certainly is not a 
state that is required and cannot be maintained across all configurations, 
architectures and compiler versions. This is the real world.

 Besides, when testing *new* code, it's even more likely to have new
 warnings, and I'd like to see as many as possible, without -Werror
 getting in the way.

I have to say that I rather like -Werror. One thing that not a lot of people 
are aware of is that you can selectively allow some warnings. -Wno-error=shadow 
would allow shadow warnings to be reported without being treated as errors.

 So I still think -Werror is fundamentally wrong in some cases, and I
 would like to pursue some approach like in my original post.
 
 BTW, for a little more context: I realize the output of 'make W=[123]'
 may not be very useful on its own, sometimes, but it's actually pretty
 useful to quickly catch potential issues in new code, by diff'ing the
 warnings in the before/after build logs. In this case, it's not helpful
 at all if the first build fails due to dubious warnings. I'm doing
 this in the context of Aiaiai [1]. Right now, I have to keep around a
 few local patches to remove -Werror from arch/{mips,sh}.

The problem is that when a kernel build throws over 125,000 warnings, it just 
becomes completely useless. That was what kind of set me off. I did wind up 
pushing this rock further up the hill than I really meant to. Still, I got the 
build under 1,400 warnings, and I now know how to address most of them in a 
systematic way.

 commit 7b9aace02b2405f0714bc08c424b72e6962f1c2e
 Author: Mark Rustad mark.d.rus...@intel.com
 Date:   Fri Aug 15 01:43:44 2014 -0700
 
compiler: Add diagnostic control macros
 
Add macros to control diagnostic messages where needed. These
are used to silence warning messages that are expected, normal
and do not indicate any sort of problem. Reducing the stream
of messages in this way helps possible problems to stand out.
 
The macros provided are:
DIAG_PUSH() - to save diagnostic settings
DIAG_POP() - to restore diagnostic settings
DIAG_IGNORE(option) - to ignore a particular warning
DIAG_GCC_IGNORE(option) - DIAG_IGNORE for gcc only
DIAG_CLANG_IGNORE(option) - DIAG_IGNORE for clang only
 
Signed-off-by: Mark Rustad mark.d.rus...@intel.com
 
 diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
 index d1e49d5..039b112 100644
 --- a/include/linux/compiler-clang.h
 +++ b/include/linux/compiler-clang.h
 @@ -10,3 +10,29 @@
 #undef uninitialized_var
 #define uninitialized_var(x) x = *((x))
 #endif
 +
 +/*
 + * Provide macros to manipulate diagnostic messages when possible.
 + * DIAG_PUSH pushes the diagnostic settings
 + * DIAG_POP pops the diagnostic settings
 + * DIAG_IGNORE(x) changes the given diagnostic setting to ignore
 + *
 + * Example:
 + *DIAG_PUSH DIAG_IGNORE(aggregate-return)
 + *struct timespec ns_to_timespec(const s64 nsec)
 + *{
 + *...
 + *}
 + *DIAG_POP
 + *
 + * Will prevent the warning on compilation of the function. Other
 + * steps are necessary to do the same thing for the call sites.
 + */
 
 While I do not want to disparage your/Mark's work here, my first thought
 about this kind of annotation is that it seems to be a pretty big burden
 to have to annotate all code with these sorts of things.

I wouldn't suggest annotating everything. However note that the annotations can 
serve as a notice that something has been analyzed and deemed ok. That can be 
useful as long as that is really true. I wouldn't take 

Re: [PATCH] misc: ti-st: st_kim.c: Cleaning up missing null-terminate after strncpy call

[Mark D Rustad]

On Aug 9, 2014, at 4:45 PM, Rickard Strandqvist 
 wrote:

> Added a guaranteed null-terminate after call to strncpy.
> 
> Signed-off-by: Rickard Strandqvist 
> ---
> drivers/misc/ti-st/st_kim.c |1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/misc/ti-st/st_kim.c b/drivers/misc/ti-st/st_kim.c
> index 9d3dbb2..bce4468 100644
> --- a/drivers/misc/ti-st/st_kim.c
> +++ b/drivers/misc/ti-st/st_kim.c
> @@ -593,6 +593,7 @@ static ssize_t store_dev_name(struct device *dev,
>   struct kim_data_s *kim_data = dev_get_drvdata(dev);
>   pr_debug("storing dev name >%s<", buf);
>   strncpy(kim_data->dev_name, buf, count);
> + kim_data->dev_name[count - 1] = '\n';

Of course this does not add termination, but adds a newline at the end of the 
buffer? Huh?

>   pr_debug("stored dev name >%s<", kim_data->dev_name);
>   return count;
> }

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] net: ethernet: myricom: myri10ge: myri10ge.c: Cleaning up missing null-terminate after strncpy call

[Mark D Rustad]

On Aug 9, 2014, at 4:40 PM, Rickard Strandqvist 
 wrote:

> Added a guaranteed null-terminate after call to strncpy.
> 
> Signed-off-by: Rickard Strandqvist 
> ---
> drivers/net/ethernet/myricom/myri10ge/myri10ge.c |1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c 
> b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> index f3d5d79..de327b6 100644
> --- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> +++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> @@ -574,6 +574,7 @@ myri10ge_validate_firmware(struct myri10ge_priv *mgp,
> 
>   /* save firmware version for ethtool */
>   strncpy(mgp->fw_version, hdr->version, sizeof(mgp->fw_version));
> + mgp->fw_version[sizeif(mgp->fw_version) - 1] = '\0';

Surely you meant sizeof above. Did you really not even compile this?

>   sscanf(mgp->fw_version, "%d.%d.%d", >fw_ver_major,
>  >fw_ver_minor, >fw_ver_tiny);

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] net: ethernet: myricom: myri10ge: myri10ge.c: Cleaning up missing null-terminate after strncpy call

[Mark D Rustad]

On Aug 9, 2014, at 4:40 PM, Rickard Strandqvist 
rickard_strandqv...@spectrumdigital.se wrote:

 Added a guaranteed null-terminate after call to strncpy.
 
 Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
 ---
 drivers/net/ethernet/myricom/myri10ge/myri10ge.c |1 +
 1 file changed, 1 insertion(+)
 
 diff --git a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c 
 b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
 index f3d5d79..de327b6 100644
 --- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
 +++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
 @@ -574,6 +574,7 @@ myri10ge_validate_firmware(struct myri10ge_priv *mgp,
 
   /* save firmware version for ethtool */
   strncpy(mgp-fw_version, hdr-version, sizeof(mgp-fw_version));
 + mgp-fw_version[sizeif(mgp-fw_version) - 1] = '\0';

Surely you meant sizeof above. Did you really not even compile this?

   sscanf(mgp-fw_version, %d.%d.%d, mgp-fw_ver_major,
  mgp-fw_ver_minor, mgp-fw_ver_tiny);

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] misc: ti-st: st_kim.c: Cleaning up missing null-terminate after strncpy call

[Mark D Rustad]

On Aug 9, 2014, at 4:45 PM, Rickard Strandqvist 
rickard_strandqv...@spectrumdigital.se wrote:

 Added a guaranteed null-terminate after call to strncpy.
 
 Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
 ---
 drivers/misc/ti-st/st_kim.c |1 +
 1 file changed, 1 insertion(+)
 
 diff --git a/drivers/misc/ti-st/st_kim.c b/drivers/misc/ti-st/st_kim.c
 index 9d3dbb2..bce4468 100644
 --- a/drivers/misc/ti-st/st_kim.c
 +++ b/drivers/misc/ti-st/st_kim.c
 @@ -593,6 +593,7 @@ static ssize_t store_dev_name(struct device *dev,
   struct kim_data_s *kim_data = dev_get_drvdata(dev);
   pr_debug(storing dev name %s, buf);
   strncpy(kim_data-dev_name, buf, count);
 + kim_data-dev_name[count - 1] = '\n';

Of course this does not add termination, but adds a newline at the end of the 
buffer? Huh?

   pr_debug(stored dev name %s, kim_data-dev_name);
   return count;
 }

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] arch: x86: ia32: ia32_aout.c: Cleaning up missing null-terminate in conjunction with strncpy

[Mark D Rustad]

Rickard,

On Jul 26, 2014, at 2:50 PM, Rickard Strandqvist 
 wrote:

> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
> And use the sizeof on the to string rather than the from string.
> 
> Signed-off-by: Rickard Strandqvist 
> ---
> arch/x86/ia32/ia32_aout.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
> index d21ff89..1a5eb43 100644
> --- a/arch/x86/ia32/ia32_aout.c
> +++ b/arch/x86/ia32/ia32_aout.c
> @@ -156,7 +156,7 @@ static int aout_core_dump(struct coredump_params *cprm)
>   fs = get_fs();
>   set_fs(KERNEL_DS);
>   has_dumped = 1;
> - strncpy(dump.u_comm, current->comm, sizeof(current->comm));
> + strlcpy(dump.u_comm, current->comm, sizeof(dump.u_comm));
>   dump.u_ar0 = offsetof(struct user32, regs);
>   dump.signal = cprm->siginfo->si_signo;
>   dump_thread32(cprm->regs, );

This patch appears to introduce an information leakage as well. The dump 
structure is on the stack and not cleared, so changing to strlcpy leaves part 
of the u_comm field holding unintialized data which is then written into the 
dump. The sizeof in the 3rd argument of the original is really an incorrect 
reference, as you corrected. A question to consider in this case: is the string 
in that structure expected to always be null-terminated or not. It is not the 
case that all such character arrays are expected to be. I don't happen to know 
in this case.

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] arch: x86: ia32: ia32_aout.c: Cleaning up missing null-terminate in conjunction with strncpy

[Mark D Rustad]

Rickard,

On Jul 26, 2014, at 2:50 PM, Rickard Strandqvist 
rickard_strandqv...@spectrumdigital.se wrote:

 Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
 And use the sizeof on the to string rather than the from string.
 
 Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
 ---
 arch/x86/ia32/ia32_aout.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
 index d21ff89..1a5eb43 100644
 --- a/arch/x86/ia32/ia32_aout.c
 +++ b/arch/x86/ia32/ia32_aout.c
 @@ -156,7 +156,7 @@ static int aout_core_dump(struct coredump_params *cprm)
   fs = get_fs();
   set_fs(KERNEL_DS);
   has_dumped = 1;
 - strncpy(dump.u_comm, current-comm, sizeof(current-comm));
 + strlcpy(dump.u_comm, current-comm, sizeof(dump.u_comm));
   dump.u_ar0 = offsetof(struct user32, regs);
   dump.signal = cprm-siginfo-si_signo;
   dump_thread32(cprm-regs, dump);

This patch appears to introduce an information leakage as well. The dump 
structure is on the stack and not cleared, so changing to strlcpy leaves part 
of the u_comm field holding unintialized data which is then written into the 
dump. The sizeof in the 3rd argument of the original is really an incorrect 
reference, as you corrected. A question to consider in this case: is the string 
in that structure expected to always be null-terminated or not. It is not the 
case that all such character arrays are expected to be. I don't happen to know 
in this case.

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] crypto: ablkcipher.c: Cleaning up missing null-terminate in conjunction with strncpy

[Mark D Rustad]

Rickard,

On Jul 26, 2014, at 7:09 AM, Rickard Strandqvist 
 wrote:

> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
> 
> Signed-off-by: Rickard Strandqvist 
> ---
> crypto/ablkcipher.c |8 
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
> index 40886c4..e446eef 100644
> --- a/crypto/ablkcipher.c
> +++ b/crypto/ablkcipher.c
> @@ -384,8 +384,8 @@ static int crypto_ablkcipher_report(struct sk_buff *skb, 
> struct crypto_alg *alg)
> {
>   struct crypto_report_blkcipher rblkcipher;
> 
> - strncpy(rblkcipher.type, "ablkcipher", sizeof(rblkcipher.type));
> - strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "",
> + strlcpy(rblkcipher.type, "ablkcipher", sizeof(rblkcipher.type));
> + strlcpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "",
>   sizeof(rblkcipher.geniv));
> 
>   rblkcipher.blocksize = alg->cra_blocksize;
> @@ -465,8 +465,8 @@ static int crypto_givcipher_report(struct sk_buff *skb, 
> struct crypto_alg *alg)
> {
>   struct crypto_report_blkcipher rblkcipher;
> 
> - strncpy(rblkcipher.type, "givcipher", sizeof(rblkcipher.type));
> - strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "",
> + strlcpy(rblkcipher.type, "givcipher", sizeof(rblkcipher.type));
> + strlcpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "",
>   sizeof(rblkcipher.geniv));
> 
>   rblkcipher.blocksize = alg->cra_blocksize;

It looks like all of these patches in the crypto area are introducing 
information leaks, including the one that first made me worry and respond. 
There were no bugs here (unless someone were to introduce a crypto method with 
a name longer than 63 characters), because stncpy fills the remainder of the 
destination with 0.

There are times when strncpy is the right function.

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] crypto: crypto_user.c: Cleaning up missing null-terminate in conjunction with strncpy

[Mark D Rustad]

Rickard,

On Jul 26, 2014, at 7:15 AM, Rickard Strandqvist 
 wrote:

> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
> 
> Signed-off-by: Rickard Strandqvist 
> ---
> crypto/crypto_user.c |   12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
> index e2a34fe..09b465e 100644
> --- a/crypto/crypto_user.c
> +++ b/crypto/crypto_user.c
> @@ -77,7 +77,7 @@ static int crypto_report_cipher(struct sk_buff *skb, struct 
> crypto_alg *alg)
> {
>   struct crypto_report_cipher rcipher;
> 
> - strncpy(rcipher.type, "cipher", sizeof(rcipher.type));
> + strlcpy(rcipher.type, "cipher", sizeof(rcipher.type));
> 
>   rcipher.blocksize = alg->cra_blocksize;
>   rcipher.min_keysize = alg->cra_cipher.cia_min_keysize;

This patch is an example of what I mentioned in my previous message. I figured 
I should go back and take a look at more of the patches. It looks to me like 
all of the changes in this patch create information leaks, because strlcpy only 
copies to the terminator, leaving the rest of the destination area in an 
otherwise uninitialized stack area alone. That structure is then copied whole 
into an skb and sent as a netlink message.

That will leak kernel information into userspace.

All of these patches need to be considered in regard to information leakage.

> @@ -96,7 +96,7 @@ static int crypto_report_comp(struct sk_buff *skb, struct 
> crypto_alg *alg)
> {
>   struct crypto_report_comp rcomp;
> 
> - strncpy(rcomp.type, "compression", sizeof(rcomp.type));
> + strlcpy(rcomp.type, "compression", sizeof(rcomp.type));
>   if (nla_put(skb, CRYPTOCFGA_REPORT_COMPRESS,
>   sizeof(struct crypto_report_comp), ))
>   goto nla_put_failure;
> @@ -109,10 +109,10 @@ nla_put_failure:
> static int crypto_report_one(struct crypto_alg *alg,
>struct crypto_user_alg *ualg, struct sk_buff *skb)
> {
> - strncpy(ualg->cru_name, alg->cra_name, sizeof(ualg->cru_name));
> - strncpy(ualg->cru_driver_name, alg->cra_driver_name,
> + strlcpy(ualg->cru_name, alg->cra_name, sizeof(ualg->cru_name));
> + strlcpy(ualg->cru_driver_name, alg->cra_driver_name,
>   sizeof(ualg->cru_driver_name));
> - strncpy(ualg->cru_module_name, module_name(alg->cra_module),
> + strlcpy(ualg->cru_module_name, module_name(alg->cra_module),
>   sizeof(ualg->cru_module_name));
> 
>   ualg->cru_type = 0;
> @@ -125,7 +125,7 @@ static int crypto_report_one(struct crypto_alg *alg,
>   if (alg->cra_flags & CRYPTO_ALG_LARVAL) {
>   struct crypto_report_larval rl;
> 
> - strncpy(rl.type, "larval", sizeof(rl.type));
> + strlcpy(rl.type, "larval", sizeof(rl.type));
>   if (nla_put(skb, CRYPTOCFGA_REPORT_LARVAL,
>   sizeof(struct crypto_report_larval), ))
>   goto nla_put_failure;

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] crypto: rng.c: Cleaning up missing null-terminate in conjunction with strncpy

[Mark D Rustad]

Rickard,

On Jul 26, 2014, at 7:18 AM, Rickard Strandqvist 
 wrote:

> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
> 
> Signed-off-by: Rickard Strandqvist 
> ---
> crypto/rng.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/crypto/rng.c b/crypto/rng.c
> index e0a25c2..c3d4fb3 100644
> --- a/crypto/rng.c
> +++ b/crypto/rng.c
> @@ -65,7 +65,7 @@ static int crypto_rng_report(struct sk_buff *skb, struct 
> crypto_alg *alg)
> {
>   struct crypto_report_rng rrng;
> 
> - strncpy(rrng.type, "rng", sizeof(rrng.type));
> + strlcpy(rrng.type, "rng", sizeof(rrng.type));
> 
>   rrng.seedsize = alg->cra_rng.seedsize;

Not to pick on this patch in particular, but you need to be careful about 
changing strncpy to strlcpy. Although strlcpy ensures termination, it does not 
prevent information leakage - strncpy ensures that the entire destination 
buffer is written. When leakage is a concern, it is better to use strncpy and 
then to store a zero in the last location of the buffer to ensure termination.

These "simple" transformations can be risky - and many of these do not 
represent any sort of problem when the source is smaller than the destination. 
I hope information leakage is being considered.

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] crypto: rng.c: Cleaning up missing null-terminate in conjunction with strncpy

[Mark D Rustad]

Rickard,

On Jul 26, 2014, at 7:18 AM, Rickard Strandqvist 
rickard_strandqv...@spectrumdigital.se wrote:

 Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
 
 Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
 ---
 crypto/rng.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/crypto/rng.c b/crypto/rng.c
 index e0a25c2..c3d4fb3 100644
 --- a/crypto/rng.c
 +++ b/crypto/rng.c
 @@ -65,7 +65,7 @@ static int crypto_rng_report(struct sk_buff *skb, struct 
 crypto_alg *alg)
 {
   struct crypto_report_rng rrng;
 
 - strncpy(rrng.type, rng, sizeof(rrng.type));
 + strlcpy(rrng.type, rng, sizeof(rrng.type));
 
   rrng.seedsize = alg-cra_rng.seedsize;

Not to pick on this patch in particular, but you need to be careful about 
changing strncpy to strlcpy. Although strlcpy ensures termination, it does not 
prevent information leakage - strncpy ensures that the entire destination 
buffer is written. When leakage is a concern, it is better to use strncpy and 
then to store a zero in the last location of the buffer to ensure termination.

These simple transformations can be risky - and many of these do not 
represent any sort of problem when the source is smaller than the destination. 
I hope information leakage is being considered.

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] crypto: crypto_user.c: Cleaning up missing null-terminate in conjunction with strncpy

[Mark D Rustad]

Rickard,

On Jul 26, 2014, at 7:15 AM, Rickard Strandqvist 
rickard_strandqv...@spectrumdigital.se wrote:

 Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
 
 Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
 ---
 crypto/crypto_user.c |   12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)
 
 diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
 index e2a34fe..09b465e 100644
 --- a/crypto/crypto_user.c
 +++ b/crypto/crypto_user.c
 @@ -77,7 +77,7 @@ static int crypto_report_cipher(struct sk_buff *skb, struct 
 crypto_alg *alg)
 {
   struct crypto_report_cipher rcipher;
 
 - strncpy(rcipher.type, cipher, sizeof(rcipher.type));
 + strlcpy(rcipher.type, cipher, sizeof(rcipher.type));
 
   rcipher.blocksize = alg-cra_blocksize;
   rcipher.min_keysize = alg-cra_cipher.cia_min_keysize;

This patch is an example of what I mentioned in my previous message. I figured 
I should go back and take a look at more of the patches. It looks to me like 
all of the changes in this patch create information leaks, because strlcpy only 
copies to the terminator, leaving the rest of the destination area in an 
otherwise uninitialized stack area alone. That structure is then copied whole 
into an skb and sent as a netlink message.

That will leak kernel information into userspace.

All of these patches need to be considered in regard to information leakage.

 @@ -96,7 +96,7 @@ static int crypto_report_comp(struct sk_buff *skb, struct 
 crypto_alg *alg)
 {
   struct crypto_report_comp rcomp;
 
 - strncpy(rcomp.type, compression, sizeof(rcomp.type));
 + strlcpy(rcomp.type, compression, sizeof(rcomp.type));
   if (nla_put(skb, CRYPTOCFGA_REPORT_COMPRESS,
   sizeof(struct crypto_report_comp), rcomp))
   goto nla_put_failure;
 @@ -109,10 +109,10 @@ nla_put_failure:
 static int crypto_report_one(struct crypto_alg *alg,
struct crypto_user_alg *ualg, struct sk_buff *skb)
 {
 - strncpy(ualg-cru_name, alg-cra_name, sizeof(ualg-cru_name));
 - strncpy(ualg-cru_driver_name, alg-cra_driver_name,
 + strlcpy(ualg-cru_name, alg-cra_name, sizeof(ualg-cru_name));
 + strlcpy(ualg-cru_driver_name, alg-cra_driver_name,
   sizeof(ualg-cru_driver_name));
 - strncpy(ualg-cru_module_name, module_name(alg-cra_module),
 + strlcpy(ualg-cru_module_name, module_name(alg-cra_module),
   sizeof(ualg-cru_module_name));
 
   ualg-cru_type = 0;
 @@ -125,7 +125,7 @@ static int crypto_report_one(struct crypto_alg *alg,
   if (alg-cra_flags  CRYPTO_ALG_LARVAL) {
   struct crypto_report_larval rl;
 
 - strncpy(rl.type, larval, sizeof(rl.type));
 + strlcpy(rl.type, larval, sizeof(rl.type));
   if (nla_put(skb, CRYPTOCFGA_REPORT_LARVAL,
   sizeof(struct crypto_report_larval), rl))
   goto nla_put_failure;

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] crypto: ablkcipher.c: Cleaning up missing null-terminate in conjunction with strncpy

[Mark D Rustad]

Rickard,

On Jul 26, 2014, at 7:09 AM, Rickard Strandqvist 
rickard_strandqv...@spectrumdigital.se wrote:

 Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
 
 Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
 ---
 crypto/ablkcipher.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)
 
 diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
 index 40886c4..e446eef 100644
 --- a/crypto/ablkcipher.c
 +++ b/crypto/ablkcipher.c
 @@ -384,8 +384,8 @@ static int crypto_ablkcipher_report(struct sk_buff *skb, 
 struct crypto_alg *alg)
 {
   struct crypto_report_blkcipher rblkcipher;
 
 - strncpy(rblkcipher.type, ablkcipher, sizeof(rblkcipher.type));
 - strncpy(rblkcipher.geniv, alg-cra_ablkcipher.geniv ?: default,
 + strlcpy(rblkcipher.type, ablkcipher, sizeof(rblkcipher.type));
 + strlcpy(rblkcipher.geniv, alg-cra_ablkcipher.geniv ?: default,
   sizeof(rblkcipher.geniv));
 
   rblkcipher.blocksize = alg-cra_blocksize;
 @@ -465,8 +465,8 @@ static int crypto_givcipher_report(struct sk_buff *skb, 
 struct crypto_alg *alg)
 {
   struct crypto_report_blkcipher rblkcipher;
 
 - strncpy(rblkcipher.type, givcipher, sizeof(rblkcipher.type));
 - strncpy(rblkcipher.geniv, alg-cra_ablkcipher.geniv ?: built-in,
 + strlcpy(rblkcipher.type, givcipher, sizeof(rblkcipher.type));
 + strlcpy(rblkcipher.geniv, alg-cra_ablkcipher.geniv ?: built-in,
   sizeof(rblkcipher.geniv));
 
   rblkcipher.blocksize = alg-cra_blocksize;

It looks like all of these patches in the crypto area are introducing 
information leaks, including the one that first made me worry and respond. 
There were no bugs here (unless someone were to introduce a crypto method with 
a name longer than 63 characters), because stncpy fills the remainder of the 
destination with 0.

There are times when strncpy is the right function.

-- 
Mark Rustad, mrus...@gmail.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH V2] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert

[Mark D Rustad]

Another nit below:

On Jul 27, 2013, at 12:32 PM, ethan.ker...@gmail.com wrote:

> Cleanup the -EINVAL return value handling and add warning message for invalid
> start,end,addr parameters.
> 
> V2: Corrected code style and tested for Linux v 3.11-rc2
> 
> Signed-off-by: ethan.zhao 
> ---
> arch/x86/pci/mmconfig-shared.c |   12 ++--
> 1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 082e881..37f6c7f 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -700,8 +700,13 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 
> start, u8 end,
>   if (!(pci_probe & PCI_PROBE_MMCONF) || pci_mmcfg_arch_init_failed)
>   return -ENODEV;
> 
> - if (start > end)
> + if (start > end || !addr) {
> + dev_warn(dev, FW_INFO
> + "Invalid address to add MMCONFIG"
> + "start %02x end %02x addr %pR\n",

As it is here, there will be no space between MMCONFIG and the word start. 
Better still, put the entire string on one line to aid those searching for the 
message. There is a general exception to the 80-column rule for user-visible 
messages such as this.



-- 
Mark Rustad, mrus...@gmail.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2] x86/PCI: MMCONFIG: cleanup and add address warning to pci_mmconfig_insert

[Mark D Rustad]

Another nit below:

On Jul 27, 2013, at 12:32 PM, ethan.ker...@gmail.com wrote:

 Cleanup the -EINVAL return value handling and add warning message for invalid
 start,end,addr parameters.
 
 V2: Corrected code style and tested for Linux v 3.11-rc2
 
 Signed-off-by: ethan.zhao ethan.z...@oracle.com
 ---
 arch/x86/pci/mmconfig-shared.c |   12 ++--
 1 files changed, 6 insertions(+), 6 deletions(-)
 
 diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
 index 082e881..37f6c7f 100644
 --- a/arch/x86/pci/mmconfig-shared.c
 +++ b/arch/x86/pci/mmconfig-shared.c
 @@ -700,8 +700,13 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 
 start, u8 end,
   if (!(pci_probe  PCI_PROBE_MMCONF) || pci_mmcfg_arch_init_failed)
   return -ENODEV;
 
 - if (start  end)
 + if (start  end || !addr) {
 + dev_warn(dev, FW_INFO
 + Invalid address to add MMCONFIG
 + start %02x end %02x addr %pR\n,

As it is here, there will be no space between MMCONFIG and the word start. 
Better still, put the entire string on one line to aid those searching for the 
message. There is a general exception to the 80-column rule for user-visible 
messages such as this.

snip

-- 
Mark Rustad, mrus...@gmail.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/