Re: [PATCH 032/141] floppy: Fix fall-through warnings for Clang
Hi,
Sorry, this was missed somehow.
I would rewrite it to something more simple instead of adding fallthrough.
What about?
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -2123,12 +2123,14 @@ static void set_floppy(int drive)
static void format_interrupt(void)
{
switch (interpret_errors()) {
+ case 0:
+ cont->done(1);
+ break;
case 1:
cont->error();
+ break;
case 2:
break;
- case 0:
- cont->done(1);
}
cont->redo();
}
On 4/20/21 11:25 PM, Gustavo A. R. Silva wrote:
> Hi all,
>
> Friendly ping: who can take this, please?
>
> Thanks
> --
> Gustavo
>
> On 11/20/20 12:28, Gustavo A. R. Silva wrote:
>> In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning
>> by explicitly adding a fallthrough pseudo-keyword in places where the
>> code is intended to fall through to the next case.
>>
>> Link: https://github.com/KSPP/linux/issues/115
>> Signed-off-by: Gustavo A. R. Silva
>> ---
>> drivers/block/floppy.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
>> index 7df79ae6b0a1..21a2a7becba0 100644
>> --- a/drivers/block/floppy.c
>> +++ b/drivers/block/floppy.c
>> @@ -2124,6 +2124,7 @@ static void format_interrupt(void)
>> switch (interpret_errors()) {
>> case 1:
>> cont->error();
>> +fallthrough;
>> case 2:
>> break;
>> case 0:
>>
Re: [PATCH] floppy: remove redundant assignment to variable st
Jens, could you please take this one? I thought to send it to you with other
cleanup patches in a merge request, but you already applied rest of the
patches. If you prefer to take it as merge request, it's ok I'll send it
based on your branch for-5.13/drivers.
On 4/15/21 4:00 PM, Colin King wrote:
> From: Colin Ian King
>
> The variable st is being assigned a value that is never read and
> it is being updated later with a new value. The initialization is
> redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King
Reviewed-by: Denis Efremov
Thanks,
Denis
> ---
> arch/x86/include/asm/floppy.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/floppy.h b/arch/x86/include/asm/floppy.h
> index d43717b423cb..6ec3fc969ad5 100644
> --- a/arch/x86/include/asm/floppy.h
> +++ b/arch/x86/include/asm/floppy.h
> @@ -74,7 +74,6 @@ static irqreturn_t floppy_hardint(int irq, void *dev_id)
> int lcount;
> char *lptr;
>
> - st = 1;
> for (lcount = virtual_dma_count, lptr = virtual_dma_addr;
>lcount; lcount--, lptr++) {
> st = inb(virtual_dma_port + FD_STATUS);
>
[PATCH 4/5] floppy: cleanups: use memcpy() to copy reply_buffer
Use memcpy() in raw_cmd_done() to copy reply_buffer instead
of a for loop.
Signed-off-by: Denis Efremov
---
drivers/block/floppy.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index c58b0b079afc..c584657bacab 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -2988,8 +2988,6 @@ static const char *drive_name(int type, int drive)
/* raw commands */
static void raw_cmd_done(int flag)
{
- int i;
-
if (!flag) {
raw_cmd->flags |= FD_RAW_FAILURE;
raw_cmd->flags |= FD_RAW_HARDFAILURE;
@@ -2997,8 +2995,7 @@ static void raw_cmd_done(int flag)
raw_cmd->reply_count = inr;
if (raw_cmd->reply_count > FD_RAW_REPLY_SIZE)
raw_cmd->reply_count = 0;
- for (i = 0; i < raw_cmd->reply_count; i++)
- raw_cmd->reply[i] = reply_buffer[i];
+ memcpy(raw_cmd->reply, reply_buffer, raw_cmd->reply_count);
if (raw_cmd->flags & (FD_RAW_READ | FD_RAW_WRITE)) {
unsigned long flags;
--
2.30.2
[PATCH 5/5] floppy: cleanups: remove FLOPPY_SILENT_DCL_CLEAR undef
FLOPPY_SILENT_DCL_CLEAR is not defined anywhere and comes from pre-git era. Just drop this undef. There is FD_SILENT_DCL_CLEAR which is really used. Signed-off-by: Denis Efremov --- drivers/block/floppy.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c index c584657bacab..678ea45f2388 100644 --- a/drivers/block/floppy.c +++ b/drivers/block/floppy.c @@ -145,8 +145,6 @@ * Better audit of register_blkdev. */ -#undef FLOPPY_SILENT_DCL_CLEAR - #define REALLY_SLOW_IO #define DEBUGT 2 -- 2.30.2
[PATCH 1/5] floppy: cleanups: remove trailing whitespaces
Cleanup trailing whitespaces as checkpatch.pl suggests.
Signed-off-by: Denis Efremov
---
include/uapi/linux/fd.h | 46 -
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/include/uapi/linux/fd.h b/include/uapi/linux/fd.h
index 8b80c63b971c..7022e3413dbc 100644
--- a/include/uapi/linux/fd.h
+++ b/include/uapi/linux/fd.h
@@ -49,11 +49,11 @@ struct floppy_struct {
#define FDCLRPRM _IO(2, 0x41)
/* clear user-defined parameters */
-#define FDSETPRM _IOW(2, 0x42, struct floppy_struct)
+#define FDSETPRM _IOW(2, 0x42, struct floppy_struct)
#define FDSETMEDIAPRM FDSETPRM
/* set user-defined parameters for current media */
-#define FDDEFPRM _IOW(2, 0x43, struct floppy_struct)
+#define FDDEFPRM _IOW(2, 0x43, struct floppy_struct)
#define FDGETPRM _IOR(2, 0x04, struct floppy_struct)
#define FDDEFMEDIAPRM FDDEFPRM
#define FDGETMEDIAPRM FDGETPRM
@@ -65,7 +65,7 @@ struct floppy_struct {
/* issue/don't issue kernel messages on media type change */
-/*
+/*
* Formatting (obsolete)
*/
#define FD_FILL_BYTE 0xF6 /* format fill byte. */
@@ -126,13 +126,13 @@ typedef char floppy_drive_name[16];
*/
struct floppy_drive_params {
signed char cmos; /* CMOS type */
-
- /* Spec2 is (HLD<<1 | ND), where HLD is head load time (1=2ms, 2=4 ms
+
+ /* Spec2 is (HLD<<1 | ND), where HLD is head load time (1=2ms, 2=4 ms
* etc) and ND is set means no DMA. Hardcoded to 6 (HLD=6ms, use DMA).
*/
unsigned long max_dtr; /* Step rate, usec */
unsigned long hlt; /* Head load/settle time, msec */
- unsigned long hut; /* Head unload time (remnant of
+ unsigned long hut; /* Head unload time (remnant of
* 8" drives) */
unsigned long srt; /* Step rate, usec */
@@ -145,12 +145,12 @@ struct floppy_drive_params {
unsigned char rps; /* rotations per second */
unsigned char tracks; /* maximum number of tracks */
unsigned long timeout; /* timeout for interrupt requests */
-
- unsigned char interleave_sect; /* if there are more sectors, use
+
+ unsigned char interleave_sect; /* if there are more sectors, use
* interleave */
-
+
struct floppy_max_errors max_errors;
-
+
char flags; /* various flags, including ftd_msg */
/*
* Announce successful media type detection and media information loss after
@@ -162,7 +162,7 @@ struct floppy_drive_params {
#define FD_BROKEN_DCL 0x20
#define FD_DEBUG 0x02
#define FD_SILENT_DCL_CLEAR 0x4
-#define FD_INVERTED_DCL 0x80 /* must be 0x80, because of hardware
+#define FD_INVERTED_DCL 0x80 /* must be 0x80, because of hardware
considerations */
char read_track;/* use readtrack during probing? */
@@ -176,8 +176,8 @@ struct floppy_drive_params {
#define FD_AUTODETECT_SIZE 8
short autodetect[FD_AUTODETECT_SIZE]; /* autodetected formats */
-
- int checkfreq; /* how often should the drive be checked for disk
+
+ int checkfreq; /* how often should the drive be checked for disk
* changes */
int native_format; /* native format of this drive */
};
@@ -225,13 +225,13 @@ struct floppy_drive_struct {
* decremented after each probe.
*/
int keep_data;
-
+
/* Prevent "aliased" accesses. */
int fd_ref;
int fd_device;
- unsigned long last_checked; /* when was the drive last checked for a
disk
+ unsigned long last_checked; /* when was the drive last checked for a
disk
* change? */
-
+
char *dmabuf;
int bufblocks;
};
@@ -255,7 +255,7 @@ enum reset_mode {
/*
* FDC state
*/
-struct floppy_fdc_state {
+struct floppy_fdc_state {
int spec1; /* spec1 value last used */
int spec2; /* spec2 value last used */
int dtr;
@@ -302,16 +302,16 @@ struct floppy_write_errors {
* to the user process are not counted.
*/
- unsigned int write_errors; /* number of physical write errors
+ unsigned int write_errors; /* number of physical write errors
* encountered */
-
+
/* position of first and last write errors */
unsigned long first_error_sector;
int first_error_generation;
unsigned long last_error_sector;
int last_error_generation;
-
- unsigned int badness; /* highest retry count for a read or write
+
+ unsigned int badness; /* highest retry count for a read or write
* operation */
};
@@ -335,7 +335,7 @@
[PATCH 2/5] floppy: cleanups: use ST0 as reply_buffer index 0
Use ST0 as 0 index for reply_buffer array. get_fdc_version() is the only
function that uses index 0 directly instead of the ST0 define.
Signed-off-by: Denis Efremov
---
drivers/block/floppy.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 960e5791d6f5..df5c32900539 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4232,7 +4232,7 @@ static char __init get_fdc_version(int fdc)
r = result(fdc);
if (r <= 0x00)
return FDC_NONE;/* No FDC present ??? */
- if ((r == 1) && (reply_buffer[0] == 0x80)) {
+ if ((r == 1) && (reply_buffer[ST0] == 0x80)) {
pr_info("FDC %d is an 8272A\n", fdc);
return FDC_8272A; /* 8272a/765 don't know DUMPREGS */
}
@@ -4257,12 +4257,12 @@ static char __init get_fdc_version(int fdc)
output_byte(fdc, FD_UNLOCK);
r = result(fdc);
- if ((r == 1) && (reply_buffer[0] == 0x80)) {
+ if ((r == 1) && (reply_buffer[ST0] == 0x80)) {
pr_info("FDC %d is a pre-1991 82077\n", fdc);
return FDC_82077_ORIG; /* Pre-1991 82077, doesn't know
* LOCK/UNLOCK */
}
- if ((r != 1) || (reply_buffer[0] != 0x00)) {
+ if ((r != 1) || (reply_buffer[ST0] != 0x00)) {
pr_info("FDC %d init: UNLOCK: unexpected return of %d bytes.\n",
fdc, r);
return FDC_UNKNOWN;
@@ -4274,11 +4274,11 @@ static char __init get_fdc_version(int fdc)
fdc, r);
return FDC_UNKNOWN;
}
- if (reply_buffer[0] == 0x80) {
+ if (reply_buffer[ST0] == 0x80) {
pr_info("FDC %d is a post-1991 82077\n", fdc);
return FDC_82077; /* Revised 82077AA passes all the tests
*/
}
- switch (reply_buffer[0] >> 5) {
+ switch (reply_buffer[ST0] >> 5) {
case 0x0:
/* Either a 82078-1 or a 82078SL running at 5Volt */
pr_info("FDC %d is an 82078.\n", fdc);
@@ -4294,7 +4294,7 @@ static char __init get_fdc_version(int fdc)
return FDC_87306;
default:
pr_info("FDC %d init: 82078 variant with unknown PARTID=%d.\n",
- fdc, reply_buffer[0] >> 5);
+ fdc, reply_buffer[ST0] >> 5);
return FDC_82078_UNKN;
}
} /* get_fdc_version */
--
2.30.2
[PATCH 3/5] floppy: cleanups: use memset() to zero reply_buffer
Use memset() to zero reply buffer in raw_cmd_copyin() instead
of a for loop.
Signed-off-by: Denis Efremov
---
drivers/block/floppy.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index df5c32900539..c58b0b079afc 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3090,7 +3090,6 @@ static int raw_cmd_copyin(int cmd, void __user *param,
{
struct floppy_raw_cmd *ptr;
int ret;
- int i;
*rcmd = NULL;
@@ -3109,8 +3108,7 @@ static int raw_cmd_copyin(int cmd, void __user *param,
if (ptr->cmd_count > FD_RAW_CMD_FULLSIZE)
return -EINVAL;
- for (i = 0; i < FD_RAW_REPLY_SIZE; i++)
- ptr->reply[i] = 0;
+ memset(ptr->reply, 0, FD_RAW_REPLY_SIZE);
ptr->resultcode = 0;
if (ptr->flags & (FD_RAW_READ | FD_RAW_WRITE)) {
--
2.30.2
[PATCH 0/5] Another small set of cleanups for floppy driver
Just a couple of patches to make checkpatch.pl a bit more happy. All these patches preserve original semantics of the code and only memset(), memcpy() patches change binary code. Denis Efremov (5): floppy: cleanups: remove trailing whitespaces floppy: cleanups: use ST0 as reply_buffer index 0 floppy: cleanups: use memset() to zero reply_buffer floppy: cleanups: use memcpy() to copy reply_buffer floppy: cleanups: remove FLOPPY_SILENT_DCL_CLEAR undef drivers/block/floppy.c | 23 +++-- include/uapi/linux/fd.h | 46 - 2 files changed, 31 insertions(+), 38 deletions(-) -- 2.30.2
Re: [PATCH] floppy: remove redundant assignment to variable st
Hi,
On 4/15/21 4:00 PM, Colin King wrote:
> From: Colin Ian King
>
> The variable st is being assigned a value that is never read and
> it is being updated later with a new value. The initialization is
> redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King
Applied, thanks!
https://github.com/evdenis/linux-floppy/commit/aeec7983d49a5f972df47c742ff8373df15b0d28
> ---
> arch/x86/include/asm/floppy.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/floppy.h b/arch/x86/include/asm/floppy.h
> index d43717b423cb..6ec3fc969ad5 100644
> --- a/arch/x86/include/asm/floppy.h
> +++ b/arch/x86/include/asm/floppy.h
> @@ -74,7 +74,6 @@ static irqreturn_t floppy_hardint(int irq, void *dev_id)
> int lcount;
> char *lptr;
>
> - st = 1;
> for (lcount = virtual_dma_count, lptr = virtual_dma_addr;
>lcount; lcount--, lptr++) {
> st = inb(virtual_dma_port + FD_STATUS);
>
[PATCH] coccinelle: misc: minmax: suppress patch generation for err returns
There is a standard idiom for "if 'ret' holds an error, return it":
return ret < 0 ? ret : 0;
Developers prefer to keep the things as they are because stylistic
change to "return min(ret, 0);" breaks readability.
Let's suppress automatic generation for this type of patches.
Signed-off-by: Denis Efremov
---
scripts/coccinelle/misc/minmax.cocci | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/scripts/coccinelle/misc/minmax.cocci
b/scripts/coccinelle/misc/minmax.cocci
index eccdd3eb3452..fcf908b34f27 100644
--- a/scripts/coccinelle/misc/minmax.cocci
+++ b/scripts/coccinelle/misc/minmax.cocci
@@ -116,16 +116,32 @@ func(...)
...>
}
+// Don't generate patches for errcode returns.
+@errcode depends on patch@
+position p;
+identifier func;
+expression x;
+binary operator cmp = {<, <=};
+@@
+
+func(...)
+{
+ <...
+ return ((x) cmp@p 0 ? (x) : 0);
+ ...>
+}
+
@pmin depends on patch@
identifier func;
expression x, y;
binary operator cmp = {<=, <};
+position p != errcode.p;
@@
func(...)
{
<...
-- ((x) cmp (y) ? (x) : (y))
+- ((x) cmp@p (y) ? (x) : (y))
+ min(x, y)
...>
}
--
2.30.2
Re: [PATCH] inotify: fix minmax.cocci warnings
On 4/7/21 8:02 PM, Julia Lawall wrote:
>
>
> On Wed, 7 Apr 2021, Jan Kara wrote:
>
>> On Tue 06-04-21 22:49:26, Julia Lawall wrote:
>>> From: kernel test robot
>>>
>>> Opportunity for min().
>>>
>>> Generated by: scripts/coccinelle/misc/minmax.cocci
>>>
>>> Fixes: 8636e3295ce3 ("coccinelle: misc: add minmax script")
>>> CC: Denis Efremov
>>> Reported-by: kernel test robot
>>> Signed-off-by: kernel test robot
>>> Signed-off-by: Julia Lawall
>> ...
>>> --- a/fs/notify/inotify/inotify_user.c
>>> +++ b/fs/notify/inotify/inotify_user.c
>>> @@ -382,7 +382,7 @@ static int inotify_add_to_idr(struct idr
>>>
>>> spin_unlock(idr_lock);
>>> idr_preload_end();
>>> - return ret < 0 ? ret : 0;
>>> + return min(ret, 0);
>>> }
>>
>> Honestly, while previous expression is a standard idiom for "if 'ret' holds
>> an error, return it", the new expression is harder to understand for me. So
>> I prefer to keep things as they are in this particular case...
>
> OK, I had doubts about it as well, but I forwarded it because I found them
> equally obscure...
>
> Denis, maybe the semantic patch should be updated to avoid this case.
No problem, I'll send an update.
Thanks,
Denis
Re: [PATCH v3] coccinelle: misc: add swap script
Ping?
On 3/5/21 1:09 PM, Denis Efremov wrote:
Check for opencoded swap() implementation.
Signed-off-by: Denis Efremov
---
Changes in v2:
- additional patch rule to drop excessive {}
- fix indentation in patch mode by anchoring ;
Changes in v3:
- Rule added for simple (without var init) swap highlighting in !patch mode
- "depends on patch && (rpvar || rp)" fixed
scripts/coccinelle/misc/swap.cocci | 122 +
1 file changed, 122 insertions(+)
create mode 100644 scripts/coccinelle/misc/swap.cocci
diff --git a/scripts/coccinelle/misc/swap.cocci
b/scripts/coccinelle/misc/swap.cocci
new file mode 100644
index ..c5e71b7ef7f5
--- /dev/null
+++ b/scripts/coccinelle/misc/swap.cocci
@@ -0,0 +1,122 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for opencoded swap() implementation.
+///
+// Confidence: High
+// Copyright: (C) 2021 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: swap
+//
+
+virtual patch
+virtual org
+virtual report
+virtual context
+
+@rvar depends on !patch@
+identifier tmp;
+expression a, b;
+type T;
+position p;
+@@
+
+(
+* T tmp;
+|
+* T tmp = 0;
+|
+* T *tmp = NULL;
+)
+... when != tmp
+* tmp = a;
+* a = b;@p
+* b = tmp;
+... when != tmp
+
+@r depends on !patch@
+identifier tmp;
+expression a, b;
+position p != rvar.p;
+@@
+
+* tmp = a;
+* a = b;@p
+* b = tmp;
+
+@rpvar depends on patch@
+identifier tmp;
+expression a, b;
+type T;
+@@
+
+(
+- T tmp;
+|
+- T tmp = 0;
+|
+- T *tmp = NULL;
+)
+... when != tmp
+- tmp = a;
+- a = b;
+- b = tmp
++ swap(a, b)
+ ;
+... when != tmp
+
+@rp depends on patch@
+identifier tmp;
+expression a, b;
+@@
+
+- tmp = a;
+- a = b;
+- b = tmp
++ swap(a, b)
+ ;
+
+@depends on patch && (rpvar || rp)@
+@@
+
+(
+ for (...;...;...)
+- {
+ swap(...);
+- }
+|
+ while (...)
+- {
+ swap(...);
+- }
+|
+ if (...)
+- {
+ swap(...);
+- }
+)
+
+
+@script:python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for swap()")
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for swap()")
+
+@script:python depends on report@
+p << rvar.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for swap()")
+
+@script:python depends on org@
+p << rvar.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for swap()")
Re: [PATCH] coccinelle: misc: update uninitialized_var.cocci documentation
Ping?
On 3/8/21 10:30 AM, Denis Efremov wrote:
> Remove the documentation link from the warning message because commit
> 3942ea7a10c9 ("deprecated.rst: Remove now removed uninitialized_var")
> removed the section from documentation. Update the rule documentation
> accordingly.
>
> Signed-off-by: Denis Efremov
> ---
> scripts/coccinelle/misc/uninitialized_var.cocci | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/coccinelle/misc/uninitialized_var.cocci
> b/scripts/coccinelle/misc/uninitialized_var.cocci
> index 8fa845cefe11..69bbaae47e73 100644
> --- a/scripts/coccinelle/misc/uninitialized_var.cocci
> +++ b/scripts/coccinelle/misc/uninitialized_var.cocci
> @@ -1,7 +1,9 @@
> // SPDX-License-Identifier: GPL-2.0-only
> ///
> /// Please, don't reintroduce uninitialized_var().
> -/// From Documentation/process/deprecated.rst:
> +///
> +/// From Documentation/process/deprecated.rst,
> +/// commit 4b19bec97c88 ("docs: deprecated.rst: Add uninitialized_var()"):
> /// For any compiler warnings about uninitialized variables, just add
> /// an initializer. Using warning-silencing tricks is dangerous as it
> /// papers over real bugs (or can in the future), and suppresses unrelated
> @@ -11,6 +13,11 @@
> /// obviously redundant, the compiler's dead-store elimination pass will
> make
> /// sure there are no needless variable writes.
> ///
> +/// Later, commit 3942ea7a10c9 ("deprecated.rst: Remove now removed
> +/// uninitialized_var") removed this section because all initializations of
> +/// this kind were cleaned-up from the kernel. This cocci rule checks that
> +/// the macro is not explicitly or implicitly reintroduced.
> +///
> // Confidence: High
> // Copyright: (C) 2020 Denis Efremov ISPRAS
> // Options: --no-includes --include-headers
> @@ -40,12 +47,10 @@ position p;
> p << r.p;
> @@
>
> -coccilib.report.print_report(p[0],
> - "WARNING this kind of initialization is deprecated
> (https://www.kernel.org/doc/html/latest/process/deprecated.html#uninitialized-var)")
> +coccilib.report.print_report(p[0], "WARNING this kind of initialization is
> deprecated")
>
> @script:python depends on org@
> p << r.p;
> @@
>
> -coccilib.org.print_todo(p[0],
> - "WARNING this kind of initialization is deprecated
> (https://www.kernel.org/doc/html/latest/process/deprecated.html#uninitialized-var)")
> +coccilib.org.print_todo(p[0], "WARNING this kind of initialization is
> deprecated")
>
Re: [PATCH] coccinelle: misc: restrict patch mode in flexible_array.cocci
Ping?
On 3/8/21 10:12 PM, Denis Efremov wrote:
> Skip patches generation for structs/unions with a single field.
> Changing a zero-length array to a flexible array member in a struct
> with no named members breaks the compilation. However, reporting
> such cases is still valuable, e.g. commit 637464c59e0b
> ("ACPI: NFIT: Fix flexible_array.cocci warnings").
>
> Signed-off-by: Denis Efremov
> ---
> scripts/coccinelle/misc/flexible_array.cocci | 23 ++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/coccinelle/misc/flexible_array.cocci
> b/scripts/coccinelle/misc/flexible_array.cocci
> index 947fbaff82a9..f427fd68ed2d 100644
> --- a/scripts/coccinelle/misc/flexible_array.cocci
> +++ b/scripts/coccinelle/misc/flexible_array.cocci
> @@ -51,21 +51,40 @@ position p : script:python() { relevant(p) };
>};
> )
>
> +@only_field depends on patch@
> +identifier name, array;
> +type T;
> +position q;
> +@@
> +
> +(
> + struct name {@q
> +T array[0];
> + };
> +|
> + struct {@q
> +T array[0];
> + };
> +)
> +
> @depends on patch@
> identifier name, array;
> type T;
> position p : script:python() { relevant(p) };
> +// position @q with rule "only_field" simplifies
> +// handling of bitfields, arrays, etc.
> +position q != only_field.q;
> @@
>
> (
> - struct name {
> + struct name {@q
> ...
> T array@p[
> - 0
> ];
>};
> |
> - struct {
> + struct {@q
> ...
> T array@p[
> - 0
>
[PATCH v5] coccinelle: misc: add minmax script
Check for opencoded min(), max() implementations.
Signed-off-by: Denis Efremov
---
Changes in v2:
- <... ...> instead of ... when any
- org mode reports fixed
- patch rule to drop excessive ()
Changes in v3:
- "depends on patch && (pmax || pmaxif || pmin || pminif)" fixed
Changes in v4:
- refarmatting rule removed
- () brackets added to the patch rules to omit excessive ones
- org/report prints changed to cycle (for p0 in p: ...)
Changes in v5:
- parentheses droppped in pminif and pmaxif rules (max_val = x ...)
scripts/coccinelle/misc/minmax.cocci | 206 +++
1 file changed, 206 insertions(+)
create mode 100644 scripts/coccinelle/misc/minmax.cocci
diff --git a/scripts/coccinelle/misc/minmax.cocci
b/scripts/coccinelle/misc/minmax.cocci
new file mode 100644
index ..eccdd3eb3452
--- /dev/null
+++ b/scripts/coccinelle/misc/minmax.cocci
@@ -0,0 +1,206 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for opencoded min(), max() implementations.
+/// Generated patches sometimes require adding a cast to fix compile warning.
+/// Warnings/patches scope intentionally limited to a function body.
+///
+// Confidence: Medium
+// Copyright: (C) 2021 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: min, max
+//
+
+
+virtual report
+virtual org
+virtual context
+virtual patch
+
+@rmax depends on !patch@
+identifier func;
+expression x, y;
+binary operator cmp = {>, >=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* ((x) cmp@p (y) ? (x) : (y))
+ ...>
+}
+
+@rmaxif depends on !patch@
+identifier func;
+expression x, y;
+expression max_val;
+binary operator cmp = {>, >=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* if ((x) cmp@p (y)) {
+* max_val = (x);
+* } else {
+* max_val = (y);
+* }
+ ...>
+}
+
+@rmin depends on !patch@
+identifier func;
+expression x, y;
+binary operator cmp = {<, <=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* ((x) cmp@p (y) ? (x) : (y))
+ ...>
+}
+
+@rminif depends on !patch@
+identifier func;
+expression x, y;
+expression min_val;
+binary operator cmp = {<, <=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* if ((x) cmp@p (y)) {
+* min_val = (x);
+* } else {
+* min_val = (y);
+* }
+ ...>
+}
+
+@pmax depends on patch@
+identifier func;
+expression x, y;
+binary operator cmp = {>=, >};
+@@
+
+func(...)
+{
+ <...
+- ((x) cmp (y) ? (x) : (y))
++ max(x, y)
+ ...>
+}
+
+@pmaxif depends on patch@
+identifier func;
+expression x, y;
+expression max_val;
+binary operator cmp = {>=, >};
+@@
+
+func(...)
+{
+ <...
+- if ((x) cmp (y)) {
+- max_val = x;
+- } else {
+- max_val = y;
+- }
++ max_val = max(x, y);
+ ...>
+}
+
+@pmin depends on patch@
+identifier func;
+expression x, y;
+binary operator cmp = {<=, <};
+@@
+
+func(...)
+{
+ <...
+- ((x) cmp (y) ? (x) : (y))
++ min(x, y)
+ ...>
+}
+
+@pminif depends on patch@
+identifier func;
+expression x, y;
+expression min_val;
+binary operator cmp = {<=, <};
+@@
+
+func(...)
+{
+ <...
+- if ((x) cmp (y)) {
+- min_val = x;
+- } else {
+- min_val = y;
+- }
++ min_val = min(x, y);
+ ...>
+}
+
+@script:python depends on report@
+p << rmax.p;
+@@
+
+for p0 in p:
+ coccilib.report.print_report(p0, "WARNING opportunity for max()")
+
+@script:python depends on org@
+p << rmax.p;
+@@
+
+for p0 in p:
+ coccilib.org.print_todo(p0, "WARNING opportunity for max()")
+
+@script:python depends on report@
+p << rmaxif.p;
+@@
+
+for p0 in p:
+ coccilib.report.print_report(p0, "WARNING opportunity for max()")
+
+@script:python depends on org@
+p << rmaxif.p;
+@@
+
+for p0 in p:
+ coccilib.org.print_todo(p0, "WARNING opportunity for max()")
+
+@script:python depends on report@
+p << rmin.p;
+@@
+
+for p0 in p:
+ coccilib.report.print_report(p0, "WARNING opportunity for min()")
+
+@script:python depends on org@
+p << rmin.p;
+@@
+
+for p0 in p:
+ coccilib.org.print_todo(p0, "WARNING opportunity for min()")
+
+@script:python depends on report@
+p << rminif.p;
+@@
+
+for p0 in p:
+ coccilib.report.print_report(p0, "WARNING opportunity for min()")
+
+@script:python depends on org@
+p << rminif.p;
+@@
+
+for p0 in p:
+ coccilib.org.print_todo(p0, "WARNING opportunity for min()")
--
2.26.2
[PATCH] coccinelle: misc: restrict patch mode in flexible_array.cocci
Skip patches generation for structs/unions with a single field.
Changing a zero-length array to a flexible array member in a struct
with no named members breaks the compilation. However, reporting
such cases is still valuable, e.g. commit 637464c59e0b
("ACPI: NFIT: Fix flexible_array.cocci warnings").
Signed-off-by: Denis Efremov
---
scripts/coccinelle/misc/flexible_array.cocci | 23 ++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/scripts/coccinelle/misc/flexible_array.cocci
b/scripts/coccinelle/misc/flexible_array.cocci
index 947fbaff82a9..f427fd68ed2d 100644
--- a/scripts/coccinelle/misc/flexible_array.cocci
+++ b/scripts/coccinelle/misc/flexible_array.cocci
@@ -51,21 +51,40 @@ position p : script:python() { relevant(p) };
};
)
+@only_field depends on patch@
+identifier name, array;
+type T;
+position q;
+@@
+
+(
+ struct name {@q
+T array[0];
+ };
+|
+ struct {@q
+T array[0];
+ };
+)
+
@depends on patch@
identifier name, array;
type T;
position p : script:python() { relevant(p) };
+// position @q with rule "only_field" simplifies
+// handling of bitfields, arrays, etc.
+position q != only_field.q;
@@
(
- struct name {
+ struct name {@q
...
T array@p[
- 0
];
};
|
- struct {
+ struct {@q
...
T array@p[
- 0
--
2.26.2
[PATCH] coccinelle: misc: update uninitialized_var.cocci documentation
Remove the documentation link from the warning message because commit
3942ea7a10c9 ("deprecated.rst: Remove now removed uninitialized_var")
removed the section from documentation. Update the rule documentation
accordingly.
Signed-off-by: Denis Efremov
---
scripts/coccinelle/misc/uninitialized_var.cocci | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/scripts/coccinelle/misc/uninitialized_var.cocci
b/scripts/coccinelle/misc/uninitialized_var.cocci
index 8fa845cefe11..69bbaae47e73 100644
--- a/scripts/coccinelle/misc/uninitialized_var.cocci
+++ b/scripts/coccinelle/misc/uninitialized_var.cocci
@@ -1,7 +1,9 @@
// SPDX-License-Identifier: GPL-2.0-only
///
/// Please, don't reintroduce uninitialized_var().
-/// From Documentation/process/deprecated.rst:
+///
+/// From Documentation/process/deprecated.rst,
+/// commit 4b19bec97c88 ("docs: deprecated.rst: Add uninitialized_var()"):
/// For any compiler warnings about uninitialized variables, just add
/// an initializer. Using warning-silencing tricks is dangerous as it
/// papers over real bugs (or can in the future), and suppresses unrelated
@@ -11,6 +13,11 @@
/// obviously redundant, the compiler's dead-store elimination pass will make
/// sure there are no needless variable writes.
///
+/// Later, commit 3942ea7a10c9 ("deprecated.rst: Remove now removed
+/// uninitialized_var") removed this section because all initializations of
+/// this kind were cleaned-up from the kernel. This cocci rule checks that
+/// the macro is not explicitly or implicitly reintroduced.
+///
// Confidence: High
// Copyright: (C) 2020 Denis Efremov ISPRAS
// Options: --no-includes --include-headers
@@ -40,12 +47,10 @@ position p;
p << r.p;
@@
-coccilib.report.print_report(p[0],
- "WARNING this kind of initialization is deprecated
(https://www.kernel.org/doc/html/latest/process/deprecated.html#uninitialized-var)")
+coccilib.report.print_report(p[0], "WARNING this kind of initialization is
deprecated")
@script:python depends on org@
p << r.p;
@@
-coccilib.org.print_todo(p[0],
- "WARNING this kind of initialization is deprecated
(https://www.kernel.org/doc/html/latest/process/deprecated.html#uninitialized-var)")
+coccilib.org.print_todo(p[0], "WARNING this kind of initialization is
deprecated")
--
2.26.2
[PATCH v4] coccinelle: misc: add minmax script
Check for opencoded min(), max() implementations.
Signed-off-by: Denis Efremov
---
Changes in v2:
- <... ...> instead of ... when any
- org mode reports fixed
- patch rule to drop excessive ()
Changes in v3:
- "depends on patch && (pmax || pmaxif || pmin || pminif)" fixed
Changes in v4:
- refarmatting rule removed
- () brackets added to the patch rules to omit excessive ones
- org/report prints changed to cycle (for p0 in p: ...)
scripts/coccinelle/misc/minmax.cocci | 206 +++
1 file changed, 206 insertions(+)
create mode 100644 scripts/coccinelle/misc/minmax.cocci
diff --git a/scripts/coccinelle/misc/minmax.cocci
b/scripts/coccinelle/misc/minmax.cocci
new file mode 100644
index ..63eeba1702ec
--- /dev/null
+++ b/scripts/coccinelle/misc/minmax.cocci
@@ -0,0 +1,206 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for opencoded min(), max() implementations.
+/// Generated patches sometimes require adding a cast to fix compile warning.
+/// Warnings/patches scope intentionally limited to a function body.
+///
+// Confidence: Medium
+// Copyright: (C) 2021 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: min, max
+//
+
+
+virtual report
+virtual org
+virtual context
+virtual patch
+
+@rmax depends on !patch@
+identifier func;
+expression x, y;
+binary operator cmp = {>, >=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* ((x) cmp@p (y) ? (x) : (y))
+ ...>
+}
+
+@rmaxif depends on !patch@
+identifier func;
+expression x, y;
+expression max_val;
+binary operator cmp = {>, >=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* if ((x) cmp@p (y)) {
+* max_val = (x);
+* } else {
+* max_val = (y);
+* }
+ ...>
+}
+
+@rmin depends on !patch@
+identifier func;
+expression x, y;
+binary operator cmp = {<, <=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* ((x) cmp@p (y) ? (x) : (y))
+ ...>
+}
+
+@rminif depends on !patch@
+identifier func;
+expression x, y;
+expression min_val;
+binary operator cmp = {<, <=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* if ((x) cmp@p (y)) {
+* min_val = (x);
+* } else {
+* min_val = (y);
+* }
+ ...>
+}
+
+@pmax depends on patch@
+identifier func;
+expression x, y;
+binary operator cmp = {>=, >};
+@@
+
+func(...)
+{
+ <...
+- ((x) cmp (y) ? (x) : (y))
++ max(x, y)
+ ...>
+}
+
+@pmaxif depends on patch@
+identifier func;
+expression x, y;
+expression max_val;
+binary operator cmp = {>=, >};
+@@
+
+func(...)
+{
+ <...
+- if ((x) cmp (y)) {
+- max_val = (x);
+- } else {
+- max_val = (y);
+- }
++ max_val = max(x, y);
+ ...>
+}
+
+@pmin depends on patch@
+identifier func;
+expression x, y;
+binary operator cmp = {<=, <};
+@@
+
+func(...)
+{
+ <...
+- ((x) cmp (y) ? (x) : (y))
++ min(x, y)
+ ...>
+}
+
+@pminif depends on patch@
+identifier func;
+expression x, y;
+expression min_val;
+binary operator cmp = {<=, <};
+@@
+
+func(...)
+{
+ <...
+- if ((x) cmp (y)) {
+- min_val = (x);
+- } else {
+- min_val = (y);
+- }
++ min_val = min(x, y);
+ ...>
+}
+
+@script:python depends on report@
+p << rmax.p;
+@@
+
+for p0 in p:
+ coccilib.report.print_report(p0, "WARNING opportunity for max()")
+
+@script:python depends on org@
+p << rmax.p;
+@@
+
+for p0 in p:
+ coccilib.org.print_todo(p0, "WARNING opportunity for max()")
+
+@script:python depends on report@
+p << rmaxif.p;
+@@
+
+for p0 in p:
+ coccilib.report.print_report(p0, "WARNING opportunity for max()")
+
+@script:python depends on org@
+p << rmaxif.p;
+@@
+
+for p0 in p:
+ coccilib.org.print_todo(p0, "WARNING opportunity for max()")
+
+@script:python depends on report@
+p << rmin.p;
+@@
+
+for p0 in p:
+ coccilib.report.print_report(p0, "WARNING opportunity for min()")
+
+@script:python depends on org@
+p << rmin.p;
+@@
+
+for p0 in p:
+ coccilib.org.print_todo(p0, "WARNING opportunity for min()")
+
+@script:python depends on report@
+p << rminif.p;
+@@
+
+for p0 in p:
+ coccilib.report.print_report(p0, "WARNING opportunity for min()")
+
+@script:python depends on org@
+p << rminif.p;
+@@
+
+for p0 in p:
+ coccilib.org.print_todo(p0, "WARNING opportunity for min()")
--
2.26.2
[PATCH 1/2] perf tests: Remove duplicate bitmap test
test_bitmap("1,3-6,8-10,24,35-37") called twice in a row.
Remove the second test.
Signed-off-by: Denis Efremov
---
tools/perf/tests/bitmap.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/perf/tests/bitmap.c b/tools/perf/tests/bitmap.c
index 96c137360918..3320613400eb 100644
--- a/tools/perf/tests/bitmap.c
+++ b/tools/perf/tests/bitmap.c
@@ -47,7 +47,6 @@ int test__bitmap_print(struct test *test __maybe_unused, int
subtest __maybe_unu
TEST_ASSERT_VAL("failed to convert map",
test_bitmap("1,3,5,7,9,11,13,15,17,19,21-40"));
TEST_ASSERT_VAL("failed to convert map", test_bitmap("2-5"));
TEST_ASSERT_VAL("failed to convert map",
test_bitmap("1,3-6,8-10,24,35-37"));
- TEST_ASSERT_VAL("failed to convert map",
test_bitmap("1,3-6,8-10,24,35-37"));
TEST_ASSERT_VAL("failed to convert map",
test_bitmap("1-10,12-20,22-30,32-40"));
return 0;
}
--
2.26.2
[PATCH 2/2] perf tests: Remove duplicate cpumap test
cpu_map_print("1,3-6,8-10,24,35-37") called twice in a row.
Remove the second test.
Signed-off-by: Denis Efremov
---
tools/perf/tests/cpumap.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
index 29c793ac7d10..f906633eae46 100644
--- a/tools/perf/tests/cpumap.c
+++ b/tools/perf/tests/cpumap.c
@@ -116,7 +116,6 @@ int test__cpu_map_print(struct test *test __maybe_unused,
int subtest __maybe_un
TEST_ASSERT_VAL("failed to convert map",
cpu_map_print("1,3,5,7,9,11,13,15,17,19,21-40"));
TEST_ASSERT_VAL("failed to convert map", cpu_map_print("2-5"));
TEST_ASSERT_VAL("failed to convert map",
cpu_map_print("1,3-6,8-10,24,35-37"));
- TEST_ASSERT_VAL("failed to convert map",
cpu_map_print("1,3-6,8-10,24,35-37"));
TEST_ASSERT_VAL("failed to convert map",
cpu_map_print("1-10,12-20,22-30,32-40"));
return 0;
}
--
2.26.2
[PATCH] sun/niu: fix wrong RXMAC_BC_FRM_CNT_COUNT count
RXMAC_BC_FRM_CNT_COUNT added to mp->rx_bcasts twice in a row in niu_xmac_interrupt(). Remove the second addition. Signed-off-by: Denis Efremov --- I don't know the code of the dirver, but this looks like a real bug. Otherwise, it's more readable as: mp->rx_bcasts += RXMAC_BC_FRM_CNT_COUNT * 2; drivers/net/ethernet/sun/niu.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c index 68695d4afacd..707ccdd03b19 100644 --- a/drivers/net/ethernet/sun/niu.c +++ b/drivers/net/ethernet/sun/niu.c @@ -3931,8 +3931,6 @@ static void niu_xmac_interrupt(struct niu *np) mp->rx_mcasts += RXMAC_MC_FRM_CNT_COUNT; if (val & XRXMAC_STATUS_RXBCAST_CNT_EXP) mp->rx_bcasts += RXMAC_BC_FRM_CNT_COUNT; - if (val & XRXMAC_STATUS_RXBCAST_CNT_EXP) - mp->rx_bcasts += RXMAC_BC_FRM_CNT_COUNT; if (val & XRXMAC_STATUS_RXHIST1_CNT_EXP) mp->rx_hist_cnt1 += RXMAC_HIST_CNT1_COUNT; if (val & XRXMAC_STATUS_RXHIST2_CNT_EXP) -- 2.26.2
[PATCH] net/hamradio/6pack: remove redundant check in sp_encaps()
"len > sp->mtu" checked twice in a row in sp_encaps().
Remove the second check.
Signed-off-by: Denis Efremov
---
drivers/net/hamradio/6pack.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index 71d6629e65c9..9f5b5614a150 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c
@@ -171,11 +171,6 @@ static void sp_encaps(struct sixpack *sp, unsigned char
*icp, int len)
goto out_drop;
}
- if (len > sp->mtu) {/* sp->mtu = AX25_MTU = max. PACLEN = 256 */
- msg = "oversized transmit packet!";
- goto out_drop;
- }
-
if (p[0] > 5) {
msg = "invalid KISS command";
goto out_drop;
--
2.26.2
[PATCH] staging: rtl8723bs: remove duplicate pstat->hwaddr check
IS_MCAST(pstat->hwaddr) checked twice in a row in
odm_RefreshRateAdaptiveMaskCE(). Remove the second check.
Signed-off-by: Denis Efremov
---
drivers/staging/rtl8723bs/hal/odm.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/staging/rtl8723bs/hal/odm.c
b/drivers/staging/rtl8723bs/hal/odm.c
index f2a9e95a1563..5e432f1bc150 100644
--- a/drivers/staging/rtl8723bs/hal/odm.c
+++ b/drivers/staging/rtl8723bs/hal/odm.c
@@ -1114,8 +1114,6 @@ void odm_RefreshRateAdaptiveMaskCE(PDM_ODM_T pDM_Odm)
if (IS_STA_VALID(pstat)) {
if (IS_MCAST(pstat->hwaddr)) /* if (psta->mac_id == 1)
*/
continue;
- if (IS_MCAST(pstat->hwaddr))
- continue;
if (true == ODM_RAStateCheck(pDM_Odm,
pstat->rssi_stat.UndecoratedSmoothedPWDB, false, &pstat->rssi_level)) {
ODM_RT_TRACE(pDM_Odm, ODM_COMP_RA_MASK,
ODM_DBG_LOUD, ("RSSI:%d, RSSI_LEVEL:%d\n",
pstat->rssi_stat.UndecoratedSmoothedPWDB, pstat->rssi_level));
--
2.26.2
[PATCH] powerpc/ptrace: Remove duplicate check from pt_regs_check()
"offsetof(struct pt_regs, msr) == offsetof(struct user_pt_regs, msr)" checked in pt_regs_check() twice in a row. Remove the second check. Signed-off-by: Denis Efremov --- arch/powerpc/kernel/ptrace/ptrace.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c index 4f3d4ff3728c..51801777906c 100644 --- a/arch/powerpc/kernel/ptrace/ptrace.c +++ b/arch/powerpc/kernel/ptrace/ptrace.c @@ -354,8 +354,6 @@ void __init pt_regs_check(void) offsetof(struct user_pt_regs, nip)); BUILD_BUG_ON(offsetof(struct pt_regs, msr) != offsetof(struct user_pt_regs, msr)); - BUILD_BUG_ON(offsetof(struct pt_regs, msr) != -offsetof(struct user_pt_regs, msr)); BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) != offsetof(struct user_pt_regs, orig_gpr3)); BUILD_BUG_ON(offsetof(struct pt_regs, ctr) != -- 2.26.2
[PATCH v3] coccinelle: misc: add minmax script
Check for opencoded min(), max() implementations.
Signed-off-by: Denis Efremov
---
Changes in v2:
- <... ...> instead of ... when any
- org mode reports fixed
- patch rule to drop excessive ()
Changes in v3:
- "depends on patch && (pmax || pmaxif || pmin || pminif)" fixed
scripts/coccinelle/misc/minmax.cocci | 224 +++
1 file changed, 224 insertions(+)
create mode 100644 scripts/coccinelle/misc/minmax.cocci
diff --git a/scripts/coccinelle/misc/minmax.cocci
b/scripts/coccinelle/misc/minmax.cocci
new file mode 100644
index ..f577f08d1e6e
--- /dev/null
+++ b/scripts/coccinelle/misc/minmax.cocci
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for opencoded min(), max() implementations.
+/// Generated patches sometimes require adding a cast to fix compile warning.
+/// Warnings/patches scope intentionally limited to a function body.
+///
+// Confidence: Medium
+// Copyright: (C) 2021 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: min, max
+//
+
+
+virtual report
+virtual org
+virtual context
+virtual patch
+
+@rmax depends on !patch@
+identifier func;
+expression x, y;
+binary operator cmp = {>, >=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* x cmp@p y ? x : y
+ ...>
+}
+
+@rmaxif depends on !patch@
+identifier func;
+expression x, y;
+expression max_val;
+binary operator cmp = {>, >=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* if (x cmp@p y) {
+* max_val = x;
+* } else {
+* max_val = y;
+* }
+ ...>
+}
+
+@rmin depends on !patch@
+identifier func;
+expression x, y;
+binary operator cmp = {<, <=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* x cmp@p y ? x : y
+ ...>
+}
+
+@rminif depends on !patch@
+identifier func;
+expression x, y;
+expression min_val;
+binary operator cmp = {<, <=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* if (x cmp@p y) {
+* min_val = x;
+* } else {
+* min_val = y;
+* }
+ ...>
+}
+
+@pmax depends on patch@
+identifier func;
+expression x, y;
+binary operator cmp = {>=, >};
+position p;
+@@
+
+func@p(...)
+{
+ <...
+- x cmp y ? x : y
++ max(x, y)
+ ...>
+}
+
+@pmaxif depends on patch@
+identifier func;
+expression x, y;
+expression max_val;
+binary operator cmp = {>=, >};
+position p;
+@@
+
+func@p(...)
+{
+ <...
+- if (x cmp y) {
+- max_val = x;
+- } else {
+- max_val = y;
+- }
++ max_val = max(x, y);
+ ...>
+}
+
+@pmin depends on patch@
+identifier func;
+expression x, y;
+binary operator cmp = {<=, <};
+position p;
+@@
+
+func@p(...)
+{
+ <...
+- x cmp y ? x : y
++ min(x, y)
+ ...>
+}
+
+@pminif depends on patch@
+identifier func;
+expression x, y;
+expression min_val;
+binary operator cmp = {<=, <};
+position p;
+@@
+
+func@p(...)
+{
+ <...
+- if (x cmp y) {
+- min_val = x;
+- } else {
+- min_val = y;
+- }
++ min_val = min(x, y);
+ ...>
+}
+
+@depends on patch && (pmax || pmaxif || pmin || pminif)@
+identifier func;
+expression x, y;
+position p;
+// FIXME: Coccinelle consumes all available ram and
+// and timeouts on every file.
+// position p = { pmin.p, pminif.p, pmax.p, pmaxif.p };
+@@
+
+func@p(...)
+{
+ <...
+(
+- (min((x), (y)))
++ min(x, y)
+|
+- (max((x), (y)))
++ max(x, y)
+)
+ ...>
+}
+
+@script:python depends on report@
+p << rmax.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for max()")
+
+@script:python depends on org@
+p << rmax.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for max()")
+
+@script:python depends on report@
+p << rmaxif.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for max()")
+
+@script:python depends on org@
+p << rmaxif.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for max()")
+
+@script:python depends on report@
+p << rmin.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for min()")
+
+@script:python depends on org@
+p << rmin.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for min()")
+
+@script:python depends on report@
+p << rminif.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for min()")
+
+@script:python depends on org@
+p << rminif.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for min()")
--
2.26.2
[PATCH v3] coccinelle: misc: add swap script
Check for opencoded swap() implementation.
Signed-off-by: Denis Efremov
---
Changes in v2:
- additional patch rule to drop excessive {}
- fix indentation in patch mode by anchoring ;
Changes in v3:
- Rule added for simple (without var init) swap highlighting in !patch mode
- "depends on patch && (rpvar || rp)" fixed
scripts/coccinelle/misc/swap.cocci | 122 +
1 file changed, 122 insertions(+)
create mode 100644 scripts/coccinelle/misc/swap.cocci
diff --git a/scripts/coccinelle/misc/swap.cocci
b/scripts/coccinelle/misc/swap.cocci
new file mode 100644
index ..c5e71b7ef7f5
--- /dev/null
+++ b/scripts/coccinelle/misc/swap.cocci
@@ -0,0 +1,122 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for opencoded swap() implementation.
+///
+// Confidence: High
+// Copyright: (C) 2021 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: swap
+//
+
+virtual patch
+virtual org
+virtual report
+virtual context
+
+@rvar depends on !patch@
+identifier tmp;
+expression a, b;
+type T;
+position p;
+@@
+
+(
+* T tmp;
+|
+* T tmp = 0;
+|
+* T *tmp = NULL;
+)
+... when != tmp
+* tmp = a;
+* a = b;@p
+* b = tmp;
+... when != tmp
+
+@r depends on !patch@
+identifier tmp;
+expression a, b;
+position p != rvar.p;
+@@
+
+* tmp = a;
+* a = b;@p
+* b = tmp;
+
+@rpvar depends on patch@
+identifier tmp;
+expression a, b;
+type T;
+@@
+
+(
+- T tmp;
+|
+- T tmp = 0;
+|
+- T *tmp = NULL;
+)
+... when != tmp
+- tmp = a;
+- a = b;
+- b = tmp
++ swap(a, b)
+ ;
+... when != tmp
+
+@rp depends on patch@
+identifier tmp;
+expression a, b;
+@@
+
+- tmp = a;
+- a = b;
+- b = tmp
++ swap(a, b)
+ ;
+
+@depends on patch && (rpvar || rp)@
+@@
+
+(
+ for (...;...;...)
+- {
+ swap(...);
+- }
+|
+ while (...)
+- {
+ swap(...);
+- }
+|
+ if (...)
+- {
+ swap(...);
+- }
+)
+
+
+@script:python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for swap()")
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for swap()")
+
+@script:python depends on report@
+p << rvar.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for swap()")
+
+@script:python depends on org@
+p << rvar.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for swap()")
--
2.26.2
[PATCH v2] coccinelle: misc: add swap script
Check for opencoded swap() implementation.
Signed-off-by: Denis Efremov
---
Changes in v2:
- additional patch rule to drop excessive {}
- fix indentation in patch mode by anchoring ;
scripts/coccinelle/misc/swap.cocci | 101 +
1 file changed, 101 insertions(+)
create mode 100644 scripts/coccinelle/misc/swap.cocci
diff --git a/scripts/coccinelle/misc/swap.cocci
b/scripts/coccinelle/misc/swap.cocci
new file mode 100644
index ..d5da9888c222
--- /dev/null
+++ b/scripts/coccinelle/misc/swap.cocci
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for opencoded swap() implementation.
+///
+// Confidence: High
+// Copyright: (C) 2021 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: swap
+//
+
+virtual patch
+virtual org
+virtual report
+virtual context
+
+@r depends on !patch@
+identifier tmp;
+expression a, b;
+type T;
+position p;
+@@
+
+(
+* T tmp;
+|
+* T tmp = 0;
+|
+* T *tmp = NULL;
+)
+... when != tmp
+* tmp = a;
+* a = b;@p
+* b = tmp;
+... when != tmp
+
+@rpvar depends on patch@
+identifier tmp;
+expression a, b;
+type T;
+@@
+
+(
+- T tmp;
+|
+- T tmp = 0;
+|
+- T *tmp = NULL;
+)
+... when != tmp
+- tmp = a;
+- a = b;
+- b = tmp
++ swap(a, b)
+ ;
+... when != tmp
+
+
+@rp depends on patch@
+identifier tmp;
+expression a, b;
+@@
+
+- tmp = a;
+- a = b;
+- b = tmp
++ swap(a, b)
+ ;
+
+@depends on (rpvar || rp)@
+@@
+
+(
+ for (...;...;...)
+- {
+ swap(...);
+- }
+|
+ while (...)
+- {
+ swap(...);
+- }
+|
+ if (...)
+- {
+ swap(...);
+- }
+)
+
+
+@script:python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for swap()")
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for swap()")
--
2.26.2
Re: [PATCH v2] coccinelle: misc: add minmax script
Sorry for wrong thread, I'll resend v2 to the right one.
Denis
On 2/19/21 12:05 PM, Denis Efremov wrote:
> Check for opencoded min(), max() implementations.
>
> Signed-off-by: Denis Efremov
> ---
>
> Changes in v2:
> - <... ...> instead of ... when any
> - org mode reports fixed
> - patch rule to drop excessive ()
>
> scripts/coccinelle/misc/minmax.cocci | 224 +++
> 1 file changed, 224 insertions(+)
> create mode 100644 scripts/coccinelle/misc/minmax.cocci
>
> diff --git a/scripts/coccinelle/misc/minmax.cocci
> b/scripts/coccinelle/misc/minmax.cocci
> new file mode 100644
> index ..61d6b61fd82c
> --- /dev/null
> +++ b/scripts/coccinelle/misc/minmax.cocci
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Check for opencoded min(), max() implementations.
> +/// Generated patches sometimes require adding a cast to fix compile warning.
> +/// Warnings/patches scope intentionally limited to a function body.
> +///
> +// Confidence: Medium
> +// Copyright: (C) 2021 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers
> +//
> +// Keywords: min, max
> +//
> +
> +
> +virtual report
> +virtual org
> +virtual context
> +virtual patch
> +
> +@rmax depends on !patch@
> +identifier func;
> +expression x, y;
> +binary operator cmp = {>, >=};
> +position p;
> +@@
> +
> +func(...)
> +{
> + <...
> +*x cmp@p y ? x : y
> + ...>
> +}
> +
> +@rmaxif depends on !patch@
> +identifier func;
> +expression x, y;
> +expression max_val;
> +binary operator cmp = {>, >=};
> +position p;
> +@@
> +
> +func(...)
> +{
> + <...
> +*if (x cmp@p y) {
> +*max_val = x;
> +*} else {
> +*max_val = y;
> +*}
> + ...>
> +}
> +
> +@rmin depends on !patch@
> +identifier func;
> +expression x, y;
> +binary operator cmp = {<, <=};
> +position p;
> +@@
> +
> +func(...)
> +{
> + <...
> +*x cmp@p y ? x : y
> + ...>
> +}
> +
> +@rminif depends on !patch@
> +identifier func;
> +expression x, y;
> +expression min_val;
> +binary operator cmp = {<, <=};
> +position p;
> +@@
> +
> +func(...)
> +{
> + <...
> +*if (x cmp@p y) {
> +*min_val = x;
> +*} else {
> +*min_val = y;
> +*}
> + ...>
> +}
> +
> +@pmax depends on patch@
> +identifier func;
> +expression x, y;
> +binary operator cmp = {>=, >};
> +position p;
> +@@
> +
> +func@p(...)
> +{
> + <...
> +-x cmp y ? x : y
> ++max(x, y)
> + ...>
> +}
> +
> +@pmaxif depends on patch@
> +identifier func;
> +expression x, y;
> +expression max_val;
> +binary operator cmp = {>=, >};
> +position p;
> +@@
> +
> +func@p(...)
> +{
> + <...
> +-if (x cmp y) {
> +-max_val = x;
> +-} else {
> +-max_val = y;
> +-}
> ++max_val = max(x, y);
> + ...>
> +}
> +
> +@pmin depends on patch@
> +identifier func;
> +expression x, y;
> +binary operator cmp = {<=, <};
> +position p;
> +@@
> +
> +func@p(...)
> +{
> + <...
> +-x cmp y ? x : y
> ++min(x, y)
> + ...>
> +}
> +
> +@pminif depends on patch@
> +identifier func;
> +expression x, y;
> +expression min_val;
> +binary operator cmp = {<=, <};
> +position p;
> +@@
> +
> +func@p(...)
> +{
> + <...
> +-if (x cmp y) {
> +-min_val = x;
> +-} else {
> +-min_val = y;
> +-}
> ++min_val = min(x, y);
> + ...>
> +}
> +
> +@depends on (pmax || pmaxif || pmin || pminif)@
> +identifier func;
> +expression x, y;
> +position p;
> +// FIXME: Coccinelle consumes all available ram and
> +// and timeouts on every file.
> +// position p = { pmin.p, pminif.p, pmax.p, pmaxif.p };
> +@@
> +
> +func@p(...)
> +{
> + <...
> +(
> +-(min((x), (y)))
> ++min(x, y)
> +|
> +-(max((x), (y)))
> ++max(x, y)
> +)
> + ...>
> +}
> +
> +@script:python depends on report@
> +p << rmax.p;
> +@@
> +
> +coccilib.report.print_report(p[0], "WARNING opportunity for max()")
> +
> +@script:python depends on org@
> +p << rmax.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "WARNING opportunity for max()")
> +
> +@script:python depends on report@
> +p << rmaxif.p;
> +@@
> +
> +coccilib.report.print_report(p[0], "WARNING opportunity for max()")
> +
> +@script:python depends on org@
> +p << rmaxif.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "WARNING opportunity for max()")
> +
> +@script:python depends on report@
> +p << rmin.p;
> +@@
> +
> +coccilib.report.print_report(p[0], "WARNING opportunity for min()")
> +
> +@script:python depends on org@
> +p << rmin.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "WARNING opportunity for min()")
> +
> +@script:python depends on report@
> +p << rminif.p;
> +@@
> +
> +coccilib.report.print_report(p[0], "WARNING opportunity for min()")
> +
> +@script:python depends on org@
> +p << rminif.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "WARNING opportunity for min()")
>
[PATCH v2] coccinelle: misc: add minmax script
Check for opencoded min(), max() implementations.
Signed-off-by: Denis Efremov
---
Changes in v2:
- <... ...> instead of ... when any
- org mode reports fixed
- patch rule to drop excessive ()
scripts/coccinelle/misc/minmax.cocci | 224 +++
1 file changed, 224 insertions(+)
create mode 100644 scripts/coccinelle/misc/minmax.cocci
diff --git a/scripts/coccinelle/misc/minmax.cocci
b/scripts/coccinelle/misc/minmax.cocci
new file mode 100644
index ..61d6b61fd82c
--- /dev/null
+++ b/scripts/coccinelle/misc/minmax.cocci
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for opencoded min(), max() implementations.
+/// Generated patches sometimes require adding a cast to fix compile warning.
+/// Warnings/patches scope intentionally limited to a function body.
+///
+// Confidence: Medium
+// Copyright: (C) 2021 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: min, max
+//
+
+
+virtual report
+virtual org
+virtual context
+virtual patch
+
+@rmax depends on !patch@
+identifier func;
+expression x, y;
+binary operator cmp = {>, >=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* x cmp@p y ? x : y
+ ...>
+}
+
+@rmaxif depends on !patch@
+identifier func;
+expression x, y;
+expression max_val;
+binary operator cmp = {>, >=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* if (x cmp@p y) {
+* max_val = x;
+* } else {
+* max_val = y;
+* }
+ ...>
+}
+
+@rmin depends on !patch@
+identifier func;
+expression x, y;
+binary operator cmp = {<, <=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* x cmp@p y ? x : y
+ ...>
+}
+
+@rminif depends on !patch@
+identifier func;
+expression x, y;
+expression min_val;
+binary operator cmp = {<, <=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* if (x cmp@p y) {
+* min_val = x;
+* } else {
+* min_val = y;
+* }
+ ...>
+}
+
+@pmax depends on patch@
+identifier func;
+expression x, y;
+binary operator cmp = {>=, >};
+position p;
+@@
+
+func@p(...)
+{
+ <...
+- x cmp y ? x : y
++ max(x, y)
+ ...>
+}
+
+@pmaxif depends on patch@
+identifier func;
+expression x, y;
+expression max_val;
+binary operator cmp = {>=, >};
+position p;
+@@
+
+func@p(...)
+{
+ <...
+- if (x cmp y) {
+- max_val = x;
+- } else {
+- max_val = y;
+- }
++ max_val = max(x, y);
+ ...>
+}
+
+@pmin depends on patch@
+identifier func;
+expression x, y;
+binary operator cmp = {<=, <};
+position p;
+@@
+
+func@p(...)
+{
+ <...
+- x cmp y ? x : y
++ min(x, y)
+ ...>
+}
+
+@pminif depends on patch@
+identifier func;
+expression x, y;
+expression min_val;
+binary operator cmp = {<=, <};
+position p;
+@@
+
+func@p(...)
+{
+ <...
+- if (x cmp y) {
+- min_val = x;
+- } else {
+- min_val = y;
+- }
++ min_val = min(x, y);
+ ...>
+}
+
+@depends on (pmax || pmaxif || pmin || pminif)@
+identifier func;
+expression x, y;
+position p;
+// FIXME: Coccinelle consumes all available ram and
+// and timeouts on every file.
+// position p = { pmin.p, pminif.p, pmax.p, pmaxif.p };
+@@
+
+func@p(...)
+{
+ <...
+(
+- (min((x), (y)))
++ min(x, y)
+|
+- (max((x), (y)))
++ max(x, y)
+)
+ ...>
+}
+
+@script:python depends on report@
+p << rmax.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for max()")
+
+@script:python depends on org@
+p << rmax.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for max()")
+
+@script:python depends on report@
+p << rmaxif.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for max()")
+
+@script:python depends on org@
+p << rmaxif.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for max()")
+
+@script:python depends on report@
+p << rmin.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for min()")
+
+@script:python depends on org@
+p << rmin.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for min()")
+
+@script:python depends on report@
+p << rminif.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for min()")
+
+@script:python depends on org@
+p << rminif.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for min()")
--
2.26.2
[PATCH v2 RESEND] coccinelle: misc: add minmax script
Check for opencoded min(), max() implementations.
Signed-off-by: Denis Efremov
---
Changes in v2:
- <... ...> instead of ... when any
- org mode reports fixed
- patch rule to drop excessive ()
scripts/coccinelle/misc/minmax.cocci | 224 +++
1 file changed, 224 insertions(+)
create mode 100644 scripts/coccinelle/misc/minmax.cocci
diff --git a/scripts/coccinelle/misc/minmax.cocci
b/scripts/coccinelle/misc/minmax.cocci
new file mode 100644
index ..61d6b61fd82c
--- /dev/null
+++ b/scripts/coccinelle/misc/minmax.cocci
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for opencoded min(), max() implementations.
+/// Generated patches sometimes require adding a cast to fix compile warning.
+/// Warnings/patches scope intentionally limited to a function body.
+///
+// Confidence: Medium
+// Copyright: (C) 2021 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: min, max
+//
+
+
+virtual report
+virtual org
+virtual context
+virtual patch
+
+@rmax depends on !patch@
+identifier func;
+expression x, y;
+binary operator cmp = {>, >=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* x cmp@p y ? x : y
+ ...>
+}
+
+@rmaxif depends on !patch@
+identifier func;
+expression x, y;
+expression max_val;
+binary operator cmp = {>, >=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* if (x cmp@p y) {
+* max_val = x;
+* } else {
+* max_val = y;
+* }
+ ...>
+}
+
+@rmin depends on !patch@
+identifier func;
+expression x, y;
+binary operator cmp = {<, <=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* x cmp@p y ? x : y
+ ...>
+}
+
+@rminif depends on !patch@
+identifier func;
+expression x, y;
+expression min_val;
+binary operator cmp = {<, <=};
+position p;
+@@
+
+func(...)
+{
+ <...
+* if (x cmp@p y) {
+* min_val = x;
+* } else {
+* min_val = y;
+* }
+ ...>
+}
+
+@pmax depends on patch@
+identifier func;
+expression x, y;
+binary operator cmp = {>=, >};
+position p;
+@@
+
+func@p(...)
+{
+ <...
+- x cmp y ? x : y
++ max(x, y)
+ ...>
+}
+
+@pmaxif depends on patch@
+identifier func;
+expression x, y;
+expression max_val;
+binary operator cmp = {>=, >};
+position p;
+@@
+
+func@p(...)
+{
+ <...
+- if (x cmp y) {
+- max_val = x;
+- } else {
+- max_val = y;
+- }
++ max_val = max(x, y);
+ ...>
+}
+
+@pmin depends on patch@
+identifier func;
+expression x, y;
+binary operator cmp = {<=, <};
+position p;
+@@
+
+func@p(...)
+{
+ <...
+- x cmp y ? x : y
++ min(x, y)
+ ...>
+}
+
+@pminif depends on patch@
+identifier func;
+expression x, y;
+expression min_val;
+binary operator cmp = {<=, <};
+position p;
+@@
+
+func@p(...)
+{
+ <...
+- if (x cmp y) {
+- min_val = x;
+- } else {
+- min_val = y;
+- }
++ min_val = min(x, y);
+ ...>
+}
+
+@depends on (pmax || pmaxif || pmin || pminif)@
+identifier func;
+expression x, y;
+position p;
+// FIXME: Coccinelle consumes all available ram and
+// and timeouts on every file.
+// position p = { pmin.p, pminif.p, pmax.p, pmaxif.p };
+@@
+
+func@p(...)
+{
+ <...
+(
+- (min((x), (y)))
++ min(x, y)
+|
+- (max((x), (y)))
++ max(x, y)
+)
+ ...>
+}
+
+@script:python depends on report@
+p << rmax.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for max()")
+
+@script:python depends on org@
+p << rmax.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for max()")
+
+@script:python depends on report@
+p << rmaxif.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for max()")
+
+@script:python depends on org@
+p << rmaxif.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for max()")
+
+@script:python depends on report@
+p << rmin.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for min()")
+
+@script:python depends on org@
+p << rmin.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for min()")
+
+@script:python depends on report@
+p << rminif.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for min()")
+
+@script:python depends on org@
+p << rminif.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for min()")
--
2.26.2
Re: [PATCH] coccinelle: misc: add swap script
On 2/18/21 2:29 PM, Julia Lawall wrote:
>
>
> On Thu, 18 Feb 2021, Denis Efremov wrote:
>
>>
>>
>> On 2/18/21 1:17 PM, Julia Lawall wrote:
>>>
>>>
>>> On Thu, 18 Feb 2021, Denis Efremov wrote:
>>>
>>>>
>>>>
>>>> On 2/18/21 12:31 AM, Julia Lawall wrote:
>>>>>> +@depends on patch@
>>>>>> +identifier tmp;
>>>>>> +expression a, b;
>>>>>> +type T;
>>>>>> +@@
>>>>>> +
>>>>>> +(
>>>>>> +- T tmp;
>>>>>> +|
>>>>>> +- T tmp = 0;
>>>>>> +|
>>>>>> +- T *tmp = NULL;
>>>>>> +)
>>>>>> +... when != tmp
>>>>>> +- tmp = a;
>>>>>> +- a = b;
>>>>>> +- b = tmp;
>>>>>> ++ swap(a, b);
>>>>>> +... when != tmp
>>>>>
>>>>> In this rule and the next one, if you remove the final ; from the b = tmp
>>>>> line and from the swap line, and put it into context code afterwards, them
>>>>> the generated code looks better on cases like fs/xfs/xfs_inode.c in the
>>>>> function xfs_lock_two_inodes where two successive swap calls are
>>>>> generated.
>>>>>
>>>>> There are also some cases such as drivers/net/wireless/ath/ath5k/phy.c in
>>>>> the function ath5k_hw_get_median_noise_floor where the swap code makes up
>>>>> a whole if branch.
>>>>
>>>>> In this cases it would be good to remove the {}.
>>>>
>>>> How this can be handled?
>>>>
>>>> If I use this pattern:
>>>> @depends on patch@
>>>> identifier tmp;
>>>> expression a, b;
>>>> @@
>>>>
>>>> (
>>>> if (...)
>>>> - {
>>>> - tmp = a;
>>>> - a = b;
>>>> - b = tmp
>>>> + swap(a, b)
>>>> ;
>>>> - }
>>>> |
>>>> - tmp = a;
>>>> - a = b;
>>>> - b = tmp
>>>> + swap(a, b)
>>>> ;
>>>> )
>>>>
>>>> The tool fails with error:
>>>>
>>>> EXN: Failure("rule starting on line 58: already tagged token:\nC code
>>>> context\nFile \"drivers/net/wireless/ath/ath5k/phy.c\", line 1574,
>>>> column 4, charpos = 41650\n around = 'sort',\n whole content =
>>>> \t\t\t\tsort[j - 1] = tmp;") in drivers/net/wireless/ath/ath5k/phy.c
>>>
>>> A disjunction basically says "at this node in the cfg, can I match the
>>> first patter, or can I match the second pattern, etc." Unfortunately in
>>> this case the two branches start matching at different nodes, so the short
>>> circuit aspect of a disjunction isn't used, and it matches both patterns.
>>>
>>> The solution is to just make two rules. The first for the if case and the
>>> second for everything else.
>>>
>>
>> if (...)
>> - {
>> - tmp = a;
>> - a = b;
>> - b = tmp
>> + swap(a, b)
>> ;
>> - }
>>
>>
>> This produces "single-line ifs".
>> Maybe generated patches can be improved somehow?
>> Moving -+; doesn't help in this case.
>
> There is clearly some problem with the management of newlines...
>
> The other alternative is to just have one rule for introducing swap and
> another for removing the braces around a swap, ie
>
> if (...)
> - {
> swap(...);
> - }
>
> I don't think it would be motivated to remove the newline in that case.
Yes, this works. I'll send v2.
Thanks
Re: [PATCH] coccinelle: misc: add swap script
On 2/18/21 1:17 PM, Julia Lawall wrote:
>
>
> On Thu, 18 Feb 2021, Denis Efremov wrote:
>
>>
>>
>> On 2/18/21 12:31 AM, Julia Lawall wrote:
>>>> +@depends on patch@
>>>> +identifier tmp;
>>>> +expression a, b;
>>>> +type T;
>>>> +@@
>>>> +
>>>> +(
>>>> +- T tmp;
>>>> +|
>>>> +- T tmp = 0;
>>>> +|
>>>> +- T *tmp = NULL;
>>>> +)
>>>> +... when != tmp
>>>> +- tmp = a;
>>>> +- a = b;
>>>> +- b = tmp;
>>>> ++ swap(a, b);
>>>> +... when != tmp
>>>
>>> In this rule and the next one, if you remove the final ; from the b = tmp
>>> line and from the swap line, and put it into context code afterwards, them
>>> the generated code looks better on cases like fs/xfs/xfs_inode.c in the
>>> function xfs_lock_two_inodes where two successive swap calls are
>>> generated.
>>>
>>> There are also some cases such as drivers/net/wireless/ath/ath5k/phy.c in
>>> the function ath5k_hw_get_median_noise_floor where the swap code makes up
>>> a whole if branch.
>>
>>> In this cases it would be good to remove the {}.
>>
>> How this can be handled?
>>
>> If I use this pattern:
>> @depends on patch@
>> identifier tmp;
>> expression a, b;
>> @@
>>
>> (
>> if (...)
>> - {
>> - tmp = a;
>> - a = b;
>> - b = tmp
>> + swap(a, b)
>> ;
>> - }
>> |
>> - tmp = a;
>> - a = b;
>> - b = tmp
>> + swap(a, b)
>> ;
>> )
>>
>> The tool fails with error:
>>
>> EXN: Failure("rule starting on line 58: already tagged token:\nC code
>> context\nFile \"drivers/net/wireless/ath/ath5k/phy.c\", line 1574,
>> column 4, charpos = 41650\n around = 'sort',\n whole content =
>> \t\t\t\tsort[j - 1] = tmp;") in drivers/net/wireless/ath/ath5k/phy.c
>
> A disjunction basically says "at this node in the cfg, can I match the
> first patter, or can I match the second pattern, etc." Unfortunately in
> this case the two branches start matching at different nodes, so the short
> circuit aspect of a disjunction isn't used, and it matches both patterns.
>
> The solution is to just make two rules. The first for the if case and the
> second for everything else.
>
if (...)
- {
- tmp = a;
- a = b;
- b = tmp
+ swap(a, b)
;
- }
This produces "single-line ifs".
Maybe generated patches can be improved somehow?
Moving -+; doesn't help in this case.
diff -u -p a/drivers/net/wireless/ath/ath9k/calib.c
b/drivers/net/wireless/ath/ath9k/calib.c
--- a/drivers/net/wireless/ath/ath9k/calib.c
+++ b/drivers/net/wireless/ath/ath9k/calib.c
@@ -32,11 +32,7 @@ static int16_t ath9k_hw_get_nf_hist_mid(
for (i = 0; i < ATH9K_NF_CAL_HIST_MAX - 1; i++) {
for (j = 1; j < ATH9K_NF_CAL_HIST_MAX - i; j++) {
- if (sort[j] > sort[j - 1]) {
- nfval = sort[j];
- sort[j] = sort[j - 1];
- sort[j - 1] = nfval;
- }
+ if (sort[j] > sort[j - 1]) swap(sort[j], sort[j - 1]);
}
}
nfval = sort[(ATH9K_NF_CAL_HIST_MAX - 1) >> 1];
diff -u -p a/drivers/net/wireless/ath/ath9k/ar9003_calib.c
b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
--- a/drivers/net/wireless/ath/ath9k/ar9003_calib.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
@@ -1011,19 +1011,11 @@ static void __ar955x_tx_iq_cal_sort(stru
for (ix = 0; ix < MAXIQCAL - 1; ix++) {
for (iy = ix + 1; iy <= MAXIQCAL - 1; iy++) {
if (coeff->mag_coeff[i][im][iy] <
- coeff->mag_coeff[i][im][ix]) {
- temp = coeff->mag_coeff[i][im][ix];
- coeff->mag_coeff[i][im][ix] =
- coeff->mag_coeff[i][im][iy];
- coeff->mag_coeff[i][im][iy] = temp;
- }
+ coeff->mag_coeff[i][im][ix])
swap(coeff->mag_coeff[i][im][ix],
+
coeff->mag_coeff[i][im][iy]);
if (coeff->phs_coeff[i][im][iy] <
- coeff->phs_coeff[i][im][ix]) {
- temp = coeff->phs_coeff[i][im][ix];
- coeff->phs_coeff[i][im][ix] =
- coeff->phs_coeff[i][im][iy];
- coeff->phs_coeff[i][im][iy] = temp;
- }
+ coeff->phs_coeff[i][im][ix])
swap(coeff->phs_coeff[i][im][ix],
+
coeff->phs_coeff[i][im][iy]);
Thanks,
Denis
Re: [PATCH] coccinelle: misc: add swap script
On 2/18/21 12:31 AM, Julia Lawall wrote:
>> +@depends on patch@
>> +identifier tmp;
>> +expression a, b;
>> +type T;
>> +@@
>> +
>> +(
>> +- T tmp;
>> +|
>> +- T tmp = 0;
>> +|
>> +- T *tmp = NULL;
>> +)
>> +... when != tmp
>> +- tmp = a;
>> +- a = b;
>> +- b = tmp;
>> ++ swap(a, b);
>> +... when != tmp
>
> In this rule and the next one, if you remove the final ; from the b = tmp
> line and from the swap line, and put it into context code afterwards, them
> the generated code looks better on cases like fs/xfs/xfs_inode.c in the
> function xfs_lock_two_inodes where two successive swap calls are
> generated.
>
> There are also some cases such as drivers/net/wireless/ath/ath5k/phy.c in
> the function ath5k_hw_get_median_noise_floor where the swap code makes up
> a whole if branch.
> In this cases it would be good to remove the {}.
How this can be handled?
If I use this pattern:
@depends on patch@
identifier tmp;
expression a, b;
@@
(
if (...)
- {
- tmp = a;
- a = b;
- b = tmp
+ swap(a, b)
;
- }
|
- tmp = a;
- a = b;
- b = tmp
+ swap(a, b)
;
)
The tool fails with error:
EXN: Failure("rule starting on line 58: already tagged token:\nC code
context\nFile \"drivers/net/wireless/ath/ath5k/phy.c\", line 1574, column 4,
charpos = 41650\n around = 'sort',\n whole content = \t\t\t\tsort[j - 1] =
tmp;") in drivers/net/wireless/ath/ath5k/phy.c
Thanks,
Denis
[PATCH] coccinelle: misc: add minmax script
Check for opencoded min(), max() implementations.
Signed-off-by: Denis Efremov
---
scripts/coccinelle/misc/minmax.cocci | 198 +++
1 file changed, 198 insertions(+)
create mode 100644 scripts/coccinelle/misc/minmax.cocci
diff --git a/scripts/coccinelle/misc/minmax.cocci
b/scripts/coccinelle/misc/minmax.cocci
new file mode 100644
index ..9ae689bb14fb
--- /dev/null
+++ b/scripts/coccinelle/misc/minmax.cocci
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for opencoded min(), max() implementations.
+/// Generated patches sometimes require adding a cast to fix compile warning.
+/// Warnings/patches scope intentionally limited to a function body.
+///
+// Confidence: Medium
+// Copyright: (C) 2021 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: min, max
+//
+
+
+virtual report
+virtual org
+virtual context
+virtual patch
+
+@rmax depends on !patch@
+identifier func;
+expression x, y;
+binary operator cmp = {>, >=};
+position p;
+@@
+
+func(...)
+{
+ ... when any
+* (x cmp y) ?@p x : y
+ ... when any
+}
+
+@rmaxif depends on !patch@
+identifier func;
+expression x, y;
+expression max_val;
+binary operator cmp = {>, >=};
+position p;
+@@
+
+func(...)
+{
+ ... when any
+* if (x cmp@p y) {
+* max_val = x;
+* } else {
+* max_val = y;
+* }
+ ... when any
+}
+
+@rmin depends on !patch@
+identifier func;
+expression x, y;
+binary operator cmp = {<, <=};
+position p;
+@@
+
+func(...)
+{
+ ... when any
+* (x cmp y) ?@p x : y
+ ... when any
+}
+
+@rminif depends on !patch@
+identifier func;
+expression x, y;
+expression min_val;
+binary operator cmp = {<, <=};
+position p;
+@@
+
+func(...)
+{
+ ... when any
+* if (x cmp@p y) {
+* min_val = x;
+* } else {
+* min_val = y;
+* }
+ ... when any
+}
+
+@depends on patch@
+identifier func;
+expression x, y;
+binary operator cmp = {>=, >};
+@@
+
+func(...)
+{
+ ... when any
+- (x cmp y) ? x : y
++ max(x, y)
+ ... when any
+}
+
+@depends on patch@
+identifier func;
+expression x, y;
+expression max_val;
+binary operator cmp = {>=, >};
+@@
+
+func(...)
+{
+ ... when any
+- if (x cmp y) {
+- max_val = x;
+- } else {
+- max_val = y;
+- }
++ max_val = max(x, y);
+ ... when any
+}
+
+@depends on patch@
+identifier func;
+expression x, y;
+binary operator cmp = {<=, <};
+@@
+
+func(...)
+{
+ ... when any
+- (x cmp y) ? x : y
++ min(x, y)
+ ... when any
+}
+
+@depends on patch@
+identifier func;
+expression x, y;
+expression min_val;
+binary operator cmp = {<=, <};
+@@
+
+func(...)
+{
+ ... when any
+- if (x cmp y) {
+- min_val = x;
+- } else {
+- min_val = y;
+- }
++ min_val = min(x, y);
+ ... when any
+}
+
+@script:python depends on report@
+p << rmax.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for max()")
+
+@script:python depends on org@
+p << rmax.p;
+@@
+
+coccilib.report.print_todo(p[0], "WARNING opportunity for max()")
+
+@script:python depends on report@
+p << rmaxif.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for max()")
+
+@script:python depends on org@
+p << rmaxif.p;
+@@
+
+coccilib.report.print_todo(p[0], "WARNING opportunity for max()")
+
+@script:python depends on report@
+p << rmin.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for min()")
+
+@script:python depends on org@
+p << rmin.p;
+@@
+
+coccilib.report.print_todo(p[0], "WARNING opportunity for min()")
+
+@script:python depends on report@
+p << rminif.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for min()")
+
+@script:python depends on org@
+p << rminif.p;
+@@
+
+coccilib.report.print_todo(p[0], "WARNING opportunity for min()")
--
2.26.2
[PATCH] coccinelle: misc: add swap script
Check for opencoded swap() implementation. Signed-off-by: Denis Efremov --- scripts/coccinelle/misc/swap.cocci | 77 ++ 1 file changed, 77 insertions(+) create mode 100644 scripts/coccinelle/misc/swap.cocci diff --git a/scripts/coccinelle/misc/swap.cocci b/scripts/coccinelle/misc/swap.cocci new file mode 100644 index ..38227a5d0855 --- /dev/null +++ b/scripts/coccinelle/misc/swap.cocci @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-2.0-only +/// +/// Check for opencoded swap() implementation. +/// +// Confidence: High +// Copyright: (C) 2021 Denis Efremov ISPRAS +// Options: --no-includes --include-headers +// +// Keywords: swap +// + +virtual patch +virtual org +virtual report +virtual context + +@r depends on !patch@ +identifier tmp; +expression a, b; +type T; +position p; +@@ + +( +* T tmp; +| +* T tmp = 0; +| +* T *tmp = NULL; +) +... when != tmp +* tmp = a; +* a = b;@p +* b = tmp; +... when != tmp + +@depends on patch@ +identifier tmp; +expression a, b; +type T; +@@ + +( +- T tmp; +| +- T tmp = 0; +| +- T *tmp = NULL; +) +... when != tmp +- tmp = a; +- a = b; +- b = tmp; ++ swap(a, b); +... when != tmp + +@depends on patch@ +identifier tmp; +expression a, b; +@@ + +- tmp = a; +- a = b; +- b = tmp; ++ swap(a, b); + +@script:python depends on report@ +p << r.p; +@@ + +coccilib.report.print_report(p[0], "WARNING opportunity for swap()") + +@script:python depends on org@ +p << r.p; +@@ + +coccilib.org.print_todo(p[0], "WARNING opportunity for swap()") -- 2.26.2
Re: [PATCH] floppy: reintroduce O_NDELAY fix
On 2/4/21 12:24 PM, Jiri Kosina wrote: > On Tue, 26 Jan 2021, Denis Efremov wrote: > >> Applied. I'll send it to Jens soon with a couple of cleanup patches. >> >> https://github.com/evdenis/linux-floppy/commit/e32f6163c47efbdbad06258560aa00d1c7e5b699 > > Denis, > > I don't see this fix in Jens' tree yet. Is there any problem with the > patch? Hi, sorry for the delay. I've just send the pull request to Jens. I tested the patch and stressed the driver with syzkaller. Everything look good with the patch to me. Thanks! Denis
[GIT PULL] Floppy patch for 5.12
The following changes since commit 0d7389718c32ad6bb8bee7895c91e2418b6b26aa: Merge tag 'nvme-5.21-2020-02-02' of git://git.infradead.org/nvme into for-5.12/drivers (2021-02-02 07:11:47 -0700) are available in the Git repository at: https://github.com/evdenis/linux-floppy tags/floppy-for-5.12 for you to fetch changes up to 8a0c014cd20516ade9654fc13b51345ec58e7be8: floppy: reintroduce O_NDELAY fix (2021-02-04 13:00:24 +0300) Floppy patch for 5.12 - O_NDELAY/O_NONBLOCK fix for floppy from Jiri Kosina. libblkid is using O_NONBLOCK when probing devices. This leads to pollution of kernel log with error messages from floppy driver. Also the driver fails a mount prior to being opened without O_NONBLOCK at least once. The patch fixes the issues. Signed-off-by: Denis Efremov Jiri Kosina (1): floppy: reintroduce O_NDELAY fix drivers/block/floppy.c | 30 +++--- 1 file changed, 15 insertions(+), 15 deletions(-)
Re: [PATCH] floppy: reintroduce O_NDELAY fix
On 1/22/21 2:13 PM, Jiri Kosina wrote:
> From: Jiri Kosina
>
> This issue was originally fixed in 09954bad4 ("floppy: refactor open()
> flags handling").
>
> The fix as a side-effect, however, introduce issue for open(O_ACCMODE)
> that is being used for ioctl-only open. I wrote a fix for that, but
> instead of it being merged, full revert of 09954bad4 was performed,
> re-introducing the O_NDELAY / O_NONBLOCK issue, and it strikes again.
>
> This is a forward-port of the original fix to current codebase; the
> original submission had the changelog below:
>
>
> Commit 09954bad4 ("floppy: refactor open() flags handling"), as a
> side-effect, causes open(/dev/fdX, O_ACCMODE) to fail. It turns out that
> this is being used setfdprm userspace for ioctl-only open().
>
> Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE)
> modes, while still keeping the original O_NDELAY bug fixed.
>
> Cc: sta...@vger.kernel.org
> Reported-by: Wim Osterholt
> Tested-by: Wim Osterholt
> Reported-and-tested-by: Kurt Garloff
> Fixes: 09954bad4 ("floppy: refactor open() flags handling")
> Fixes: f2791e7ead ("Revert "floppy: refactor open() flags handling"")
> Signed-off-by: Jiri Kosina
Applied. I'll send it to Jens soon with a couple of cleanup patches.
https://github.com/evdenis/linux-floppy/commit/e32f6163c47efbdbad06258560aa00d1c7e5b699
Thanks,
Denis
> ---
> drivers/block/floppy.c | 30 +++---
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index dfe1dfc901cc..0b71292d9d5a 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4121,23 +4121,23 @@ static int floppy_open(struct block_device *bdev,
> fmode_t mode)
> if (fdc_state[FDC(drive)].rawcmd == 1)
> fdc_state[FDC(drive)].rawcmd = 2;
>
> - if (!(mode & FMODE_NDELAY)) {
> - if (mode & (FMODE_READ|FMODE_WRITE)) {
> - drive_state[drive].last_checked = 0;
> - clear_bit(FD_OPEN_SHOULD_FAIL_BIT,
> - &drive_state[drive].flags);
> - if (bdev_check_media_change(bdev))
> - floppy_revalidate(bdev->bd_disk);
> - if (test_bit(FD_DISK_CHANGED_BIT,
> &drive_state[drive].flags))
> - goto out;
> - if (test_bit(FD_OPEN_SHOULD_FAIL_BIT,
> &drive_state[drive].flags))
> - goto out;
> - }
> - res = -EROFS;
> - if ((mode & FMODE_WRITE) &&
> - !test_bit(FD_DISK_WRITABLE_BIT, &drive_state[drive].flags))
> + if (mode & (FMODE_READ|FMODE_WRITE)) {
> + drive_state[drive].last_checked = 0;
> + clear_bit(FD_OPEN_SHOULD_FAIL_BIT, &drive_state[drive].flags);
> + if (bdev_check_media_change(bdev))
> + floppy_revalidate(bdev->bd_disk);
> + if (test_bit(FD_DISK_CHANGED_BIT, &drive_state[drive].flags))
> + goto out;
> + if (test_bit(FD_OPEN_SHOULD_FAIL_BIT,
> &drive_state[drive].flags))
> goto out;
> }
> +
> + res = -EROFS;
> +
> + if ((mode & FMODE_WRITE) &&
> + !test_bit(FD_DISK_WRITABLE_BIT,
> &drive_state[drive].flags))
> + goto out;
> +
> mutex_unlock(&open_lock);
> mutex_unlock(&floppy_mutex);
> return 0;
>
Re: [PATCH] floppy: reintroduce O_NDELAY fix
On 1/26/21 12:31 PM, Kurt Garloff wrote:
> Hi Denis, Jiri, Jens,
>
> Am 26.01.21 um 09:21 schrieb Denis Efremov:
>> On 1/22/21 2:13 PM, Jiri Kosina wrote:
>>> From: Jiri Kosina
>>>
>>> This issue was originally fixed in 09954bad4 ("floppy: refactor open()
>>> flags handling").
>>>
>>> The fix as a side-effect, however, introduce issue for open(O_ACCMODE)
>>> that is being used for ioctl-only open. I wrote a fix for that, but
>>> instead of it being merged, full revert of 09954bad4 was performed,
>>> re-introducing the O_NDELAY / O_NONBLOCK issue, and it strikes again.
>>>
>>> This is a forward-port of the original fix to current codebase; the
>>> original submission had the changelog below:
>>>
>>>
>>> Commit 09954bad4 ("floppy: refactor open() flags handling"), as a
>>> side-effect, causes open(/dev/fdX, O_ACCMODE) to fail. It turns out that
>>> this is being used setfdprm userspace for ioctl-only open().
>>>
>>> Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE)
>>> modes, while still keeping the original O_NDELAY bug fixed.
>>>
>>> Cc: sta...@vger.kernel.org
>>> Reported-by: Wim Osterholt
>>> Tested-by: Wim Osterholt
>>> Reported-and-tested-by: Kurt Garloff
>>> Fixes: 09954bad4 ("floppy: refactor open() flags handling")
>>> Fixes: f2791e7ead ("Revert "floppy: refactor open() flags handling"")
>>> Signed-off-by: Jiri Kosina
>> Applied. I'll send it to Jens soon with a couple of cleanup patches.
>>
>> https://github.com/evdenis/linux-floppy/commit/e32f6163c47efbdbad06258560aa00d1c7e5b699
>
> Great, thanks.
>
> Due to libblkid (rightfully) using O_NONBLOCK these days when probing
> devices, the floppy driver does spit loads of
> [ 9.533513] floppy0: disk absent or changed during operation
> [ 9.534989] blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ)
> flags 0x0 phys_seg 1 prio class 0
> [ 9.537206] Buffer I/O error on dev fd0, logical block 0, async page read
> [ 9.546837] floppy0: disk absent or changed during operation
> [ 9.548389] blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ)
> flags 0x80700 phys_seg 1
> and fails a mount prior to being opened without O_NONBLOCK at least once.
> (Reproduction is easy with qemu-kvm.)
>
> The patch addresses it and I would suggest it to also be backported and
> applied to the active stable kernel trees.
Yes, it will be backported to all stable trees including v4.4
Thanks,
Denis
Re: [PATCH v1] trace: Fix race in trace_open and buffer resize call
On 1/22/21 5:37 PM, Steven Rostedt wrote: > On Fri, 22 Jan 2021 16:55:29 +0530 > Gaurav Kohli wrote: > That could possibly work. >> >> Yes, this will work, As i have tested similar patch for internal testing >> for kernel branches like 5.4/4.19. > > Can you or Denis send a proper patch for Greg to backport? I'll review it, > test it and give my ack to it, so Greg can take it without issue. > I can prepare the patch, but it will be compile-tested only from my side. Honestly, I think it's better when the patch and its backports have the same author and commit message. And I can't test the fix by myself as I don't know how to reproduce conditions for the bug. I think it's better if Gaurav will prepare this backport, unless he have reasons for me to do it or maybe just don't have enough time nowadays. Gaurav, if you want to somehow mention me you add my Reported-by: Thanks, Denis
Re: [PATCH v1] trace: Fix race in trace_open and buffer resize call
On 1/21/21 10:09 PM, Steven Rostedt wrote:
> On Thu, 21 Jan 2021 17:30:40 +0300
> Denis Efremov wrote:
>
>> Hi,
>>
>> This patch (CVE-2020-27825) was tagged with
>> Fixes: b23d7a5f4a07a ("ring-buffer: speed up buffer resets by avoiding
>> synchronize_rcu for each CPU")
>>
>> I'm not an expert here but it seems like b23d7a5f4a07a only refactored
>> ring_buffer_reset_cpu() by introducing reset_disabled_cpu_buffer() without
>> significant changes. Hence,
>> mutex_lock(&buffer->mutex)/mutex_unlock(&buffer->mutex)
>> can be backported further than b23d7a5f4a07a~ and to all LTS kernels. Is
>> b23d7a5f4a07a the actual cause of the bug?
>>
>
> Ug, that looks to be a mistake. Looking back at the thread about this:
>
>
> https://lore.kernel.org/linux-arm-msm/20200915141304.41fa7...@gandalf.local.home/
I see from the link that it was planned to backport the patch to LTS kernels:
> Actually we are seeing issue in older kernel like 4.19/4.14/5.4 and there
> below patch was not
> present in stable branches:
> Commit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by avoiding
> synchronize_rcu for each CPU")
The point is that it's not backported yet. Maybe because of Fixes tag. I've
discovered
this while trying to formalize CVE-2020-27825 bug in cvehound
https://github.com/evdenis/cvehound/blob/master/cvehound/cve/CVE-2020-27825.cocci
I think that the backport to the 4.4+ should be something like:
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 547a3a5ac57b..2171b377bbc1 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -4295,6 +4295,8 @@ void ring_buffer_reset_cpu(struct ring_buffer *buffer,
int cpu)
if (!cpumask_test_cpu(cpu, buffer->cpumask))
return;
+ mutex_lock(&buffer->mutex);
+
atomic_inc(&buffer->resize_disabled);
atomic_inc(&cpu_buffer->record_disabled);
@@ -4317,6 +4319,8 @@ void ring_buffer_reset_cpu(struct ring_buffer *buffer,
int cpu)
atomic_dec(&cpu_buffer->record_disabled);
atomic_dec(&buffer->resize_disabled);
+
+ mutex_unlock(&buffer->mutex);
}
EXPORT_SYMBOL_GPL(ring_buffer_reset_cpu);
Thanks,
Denis
Re: [PATCH RESEND] floppy: fix open(O_ACCMODE) for ioctl-only open
On 1/21/21 5:44 PM, Jiri Kosina wrote:
> On Thu, 21 Jan 2021, Jiri Kosina wrote:
>
>> I am currently waiting for confirmation by the original reporter that the
>> patch below fixes the issue.
>
> ... a now a patch that actually compiles :) (made a mistake when
> forward-porting from the older kernel on which this has been reported).
Oh, sorry for the last message (forgot to check the inbox before hitting
the send button). I'll test the patch. A couple of nitpicks below.
>
> From: Jiri Kosina
> Subject: [PATCH v2] floppy: reintroduce O_NDELAY fix
>
> Originally fixed in 09954bad4 ("floppy: refactor open() flags handling")
> then reverted for unknown reason in f2791e7eadf437 instead of taking
> the open(O_ACCMODE) for ioctl-only open fix, which had the changelog below
>
>
> Commit 09954bad4 ("floppy: refactor open() flags handling"), as a
> side-effect, causes open(/dev/fdX, O_ACCMODE) to fail. It turns out that
> this is being used setfdprm userspace for ioctl-only open().
>
> Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE)
> modes, while still keeping the original O_NDELAY bug fixed.
>
> Cc: sta...@vger.kernel.org # v4.5+
Are you sure that it's not worth to backport it to LTS v4.4?
Because f2791e7ead is just a revert and 09954bad4 is not
presented in v4.4 I'm not sure what fixes tag is better to
use in this case.
> Reported-by: Wim Osterholt
> Tested-by: Wim Osterholt
> Signed-off-by: Jiri Kosina
> =
>
> Fixes: 09954bad4 ("floppy: refactor open() flags handling")
> Fixes: f2791e7ead ("Revert "floppy: refactor open() flags handling"")
> Signed-off-by: Jiri Kosina
> ---
>
> v1 -> v2: fix build issue due to bad forward-port
>
> drivers/block/floppy.c | 30 +++---
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index dfe1dfc901cc..f9e839c8c5aa 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4121,23 +4121,23 @@ static int floppy_open(struct block_device *bdev,
> fmode_t mode)
> if (fdc_state[FDC(drive)].rawcmd == 1)
> fdc_state[FDC(drive)].rawcmd = 2;
>
> - if (!(mode & FMODE_NDELAY)) {
> - if (mode & (FMODE_READ|FMODE_WRITE)) {
> - drive_state[drive].last_checked = 0;
> - clear_bit(FD_OPEN_SHOULD_FAIL_BIT,
> - &drive_state[drive].flags);
> - if (bdev_check_media_change(bdev))
> - floppy_revalidate(bdev->bd_disk);
> - if (test_bit(FD_DISK_CHANGED_BIT,
> &drive_state[drive].flags))
> - goto out;
> - if (test_bit(FD_OPEN_SHOULD_FAIL_BIT,
> &drive_state[drive].flags))
> - goto out;
> - }
> - res = -EROFS;
> - if ((mode & FMODE_WRITE) &&
> - !test_bit(FD_DISK_WRITABLE_BIT, &drive_state[drive].flags))
> + if (mode & (FMODE_READ|FMODE_WRITE)) {
> + UDRS->last_checked = 0;
UDRS will still break the compilation here.
> + clear_bit(FD_OPEN_SHOULD_FAIL_BIT, &drive_state[drive].flags);
> + if (bdev_check_media_change(bdev))
> + floppy_revalidate(bdev->bd_disk);
> + if (test_bit(FD_DISK_CHANGED_BIT, &drive_state[drive].flags))
> + goto out;
> + if (test_bit(FD_OPEN_SHOULD_FAIL_BIT,
> &drive_state[drive].flags))
> goto out;
> }
> +
> + res = -EROFS;
> +
> + if ((mode & FMODE_WRITE) &&
> + !test_bit(FD_DISK_WRITABLE_BIT,
> &drive_state[drive].flags))
> + goto out;
> +
> mutex_unlock(&open_lock);
> mutex_unlock(&floppy_mutex);
> return 0;
>
Re: [PATCH RESEND] floppy: fix open(O_ACCMODE) for ioctl-only open
On 1/21/21 1:25 PM, Jiri Kosina wrote:
> On Thu, 21 Jan 2021, Denis Efremov wrote:
>
>> I think it's hard to recall the exact reasons after so many years.
>
> Yeah, I guess so :)
>
>> I'll send a patch today based on this one.
>
> I am currently waiting for confirmation by the original reporter that the
> patch below fixes the issue.
>
>
>
> From: Jiri Kosina
> Subject: [PATCH] floppy: reintroduce O_NDELAY fix
>
> Originally fixed in 09954bad4 ("floppy: refactor open() flags handling")
> then reverted for unknown reason in f2791e7eadf437 instead of taking
> the open(O_ACCMODE) for ioctl-only open fix, which had the changelog below
>
>
> Commit 09954bad4 ("floppy: refactor open() flags handling"), as a
> side-effect, causes open(/dev/fdX, O_ACCMODE) to fail. It turns out that
> this is being used setfdprm userspace for ioctl-only open().
>
> Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE)
> modes, while still keeping the original O_NDELAY bug fixed.
>
> Cc: sta...@vger.kernel.org # v4.5+
> Reported-by: Wim Osterholt
> Tested-by: Wim Osterholt
> Signed-off-by: Jiri Kosina
> =
>
> Fixes: 09954bad4 ("floppy: refactor open() flags handling")
> Fixes: f2791e7ead ("Revert "floppy: refactor open() flags handling"")
> Signed-off-by: Jiri Kosina
> ---
> drivers/block/floppy.c | 29 ++---
> 1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index dfe1dfc901cc..bda9417aa0a8 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4121,23 +4121,22 @@ static int floppy_open(struct block_device *bdev,
> fmode_t mode)
> if (fdc_state[FDC(drive)].rawcmd == 1)
> fdc_state[FDC(drive)].rawcmd = 2;
>
> - if (!(mode & FMODE_NDELAY)) {
> - if (mode & (FMODE_READ|FMODE_WRITE)) {
> - drive_state[drive].last_checked = 0;
> - clear_bit(FD_OPEN_SHOULD_FAIL_BIT,
> - &drive_state[drive].flags);
> - if (bdev_check_media_change(bdev))
> - floppy_revalidate(bdev->bd_disk);
> - if (test_bit(FD_DISK_CHANGED_BIT,
> &drive_state[drive].flags))
> - goto out;
> - if (test_bit(FD_OPEN_SHOULD_FAIL_BIT,
> &drive_state[drive].flags))
> - goto out;
> - }
> - res = -EROFS;
> - if ((mode & FMODE_WRITE) &&
> - !test_bit(FD_DISK_WRITABLE_BIT, &drive_state[drive].flags))
> + if (mode & (FMODE_READ|FMODE_WRITE)) {
As the bot points out this was refactored a bit in:
8d9d34e25a37 ("floppy: cleanup: expand macro UDRS")
4a6f3d480edc ("floppy: use bdev_check_media_change")
Should be something like:
+ drive_state[drive].last_checked = 0;
+ clear_bit(FD_OPEN_SHOULD_FAIL_BIT,
+ &drive_state[drive].flags);
+ if (bdev_check_media_change(bdev))
+ floppy_revalidate(bdev->bd_disk);
> + if (test_bit(FD_DISK_CHANGED_BIT, &drive_state[drive].flags))
> + goto out;
> + if (test_bit(FD_OPEN_SHOULD_FAIL_BIT,
> &drive_state[drive].flags))
> goto out;
> }
> +
> + res = -EROFS;
> +
> + if ((mode & FMODE_WRITE) &&
> + !test_bit(FD_DISK_WRITABLE_BIT,
> &drive_state[drive].flags))
> + goto out;
> +
> mutex_unlock(&open_lock);
> mutex_unlock(&floppy_mutex);
> return 0;
>
Re: [PATCH v1] trace: Fix race in trace_open and buffer resize call
Hi,
This patch (CVE-2020-27825) was tagged with
Fixes: b23d7a5f4a07a ("ring-buffer: speed up buffer resets by avoiding
synchronize_rcu for each CPU")
I'm not an expert here but it seems like b23d7a5f4a07a only refactored
ring_buffer_reset_cpu() by introducing reset_disabled_cpu_buffer() without
significant changes. Hence,
mutex_lock(&buffer->mutex)/mutex_unlock(&buffer->mutex)
can be backported further than b23d7a5f4a07a~ and to all LTS kernels. Is
b23d7a5f4a07a the actual cause of the bug?
Thanks,
Denis
On 10/6/20 12:33 PM, Gaurav Kohli wrote:
> Below race can come, if trace_open and resize of
> cpu buffer is running parallely on different cpus
> CPUXCPUY
> ring_buffer_resize
> atomic_read(&buffer->resize_disabled)
> tracing_open
> tracing_reset_online_cpus
> ring_buffer_reset_cpu
> rb_reset_cpu
> rb_update_pages
> remove/insert pages
> resetting pointer
>
> This race can cause data abort or some times infinte loop in
> rb_remove_pages and rb_insert_pages while checking pages
> for sanity.
>
> Take buffer lock to fix this.
>
> Signed-off-by: Gaurav Kohli
> Cc: sta...@vger.kernel.org
> ---
> Changes since v0:
> -Addressed Steven's review comments.
>
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 93ef0ab..15bf28b 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -4866,6 +4866,9 @@ void ring_buffer_reset_cpu(struct trace_buffer *buffer,
> int cpu)
> if (!cpumask_test_cpu(cpu, buffer->cpumask))
> return;
>
> + /* prevent another thread from changing buffer sizes */
> + mutex_lock(&buffer->mutex);
> +
> atomic_inc(&cpu_buffer->resize_disabled);
> atomic_inc(&cpu_buffer->record_disabled);
>
> @@ -4876,6 +4879,8 @@ void ring_buffer_reset_cpu(struct trace_buffer *buffer,
> int cpu)
>
> atomic_dec(&cpu_buffer->record_disabled);
> atomic_dec(&cpu_buffer->resize_disabled);
> +
> + mutex_unlock(&buffer->mutex);
> }
> EXPORT_SYMBOL_GPL(ring_buffer_reset_cpu);
>
> @@ -4889,6 +4894,9 @@ void ring_buffer_reset_online_cpus(struct trace_buffer
> *buffer)
> struct ring_buffer_per_cpu *cpu_buffer;
> int cpu;
>
> + /* prevent another thread from changing buffer sizes */
> + mutex_lock(&buffer->mutex);
> +
> for_each_online_buffer_cpu(buffer, cpu) {
> cpu_buffer = buffer->buffers[cpu];
>
> @@ -4907,6 +4915,8 @@ void ring_buffer_reset_online_cpus(struct trace_buffer
> *buffer)
> atomic_dec(&cpu_buffer->record_disabled);
> atomic_dec(&cpu_buffer->resize_disabled);
> }
> +
> + mutex_unlock(&buffer->mutex);
> }
>
> /**
>
Re: [PATCH RESEND] floppy: fix open(O_ACCMODE) for ioctl-only open
Hi,
On 1/19/21 6:53 PM, Jiri Kosina wrote:
> On Mon, 25 Jul 2016, Jens Axboe wrote:
>
>>> From: Jiri Kosina
>>>
>>> Commit 09954bad4 ("floppy: refactor open() flags handling"), as a
>>> side-effect, causes open(/dev/fdX, O_ACCMODE) to fail. It turns out that
>>> this is being used setfdprm userspace for ioctl-only open().
>>>
>>> Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE)
>>> modes, while still keeping the original O_NDELAY bug fixed.
>>>
>>> Cc: sta...@vger.kernel.org # v4.5+
>>> Reported-by: Wim Osterholt
>>> Tested-by: Wim Osterholt
>>> Signed-off-by: Jiri Kosina
>>
>> Added for this series, thanks.
>
> [ CCing Denis too ]
>
> Let me revive this 4 years old thread.
>
> I've just now noticed that instead of my patch above being merged, what
> happened instead was
>
> commit f2791e7eadf437633f30faa51b30878cf15650be
> Author: Jens Axboe
> Date: Thu Aug 25 08:56:51 2016 -0600
>
> Revert "floppy: refactor open() flags handling"
>
> This reverts commit 09954bad448791ef01202351d437abdd9497a804.
>
>
> which was plain revert of 09954bad4 (without any further explanation),
> which in turn reintroduced the O_NDELAY issue, and I've just been hit by
> it again.
>
> I am not able to find any e-mail thread that'd indicate why ultimately
> revert happened, instead of mergin my fix.
I think it's hard to recall the exact reasons after so many years.
I'll send a patch today based on this one.
Best Regards,
Denis
[PATCH v8] coccinelle: api: add kfree_mismatch script
Check that alloc and free types of functions match each other.
Signed-off-by: Denis Efremov
---
Changes in v2:
- Lines are limited to 80 characters where possible
- Confidence changed from High to Medium because of
fs/btrfs/send.c:1119 false-positive
- __vmalloc_area_node() explicitly excluded from analysis
instead of !(file in "mm/vmalloc.c") condition
Changes in v3:
- prints style in org && report modes changed for python2
Changes in v4:
- missing msg argument to print_todo fixed
Changes in v5:
- fix position p in kfree rule
- move @kok and @v positions in choice rule after the arguments
- remove kvmalloc suggestions
Changes in v6:
- more asterisks added in context mode
- second @kok added to the choice rule
Changes in v7:
- file renamed to kfree_mismatch.cocci
- python function relevant() removed
- additional rule for filtering free positions added
- btrfs false-positive fixed
- confidence level changed to high
- kvfree_switch rule added
- names for position variables changed to @a (alloc) and @f (free)
Changes in v8:
- kzfree() replaced with kfree_sensitive()
- "position f != free.fok;" simplified to "position f;" in patch
and kvfree_switch rules
scripts/coccinelle/api/kfree_mismatch.cocci | 229
1 file changed, 229 insertions(+)
create mode 100644 scripts/coccinelle/api/kfree_mismatch.cocci
diff --git a/scripts/coccinelle/api/kfree_mismatch.cocci
b/scripts/coccinelle/api/kfree_mismatch.cocci
new file mode 100644
index ..843b794fac7b
--- /dev/null
+++ b/scripts/coccinelle/api/kfree_mismatch.cocci
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check that kvmalloc'ed memory is freed by kfree functions,
+/// vmalloc'ed by vfree functions and kvmalloc'ed by kvfree
+/// functions.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual patch
+virtual report
+virtual org
+virtual context
+
+@alloc@
+expression E, E1;
+position kok, vok;
+@@
+
+(
+ if (...) {
+...
+E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|
+ kmalloc_node\|kzalloc_node\|kmalloc_array\|
+ kmalloc_array_node\|kcalloc_node\)(...)@kok
+...
+ } else {
+...
+E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|
+ vzalloc_node\|vmalloc_exec\|vmalloc_32\|
+ vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|
+ __vmalloc_node\)(...)@vok
+...
+ }
+|
+ E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(...)@kok
+ ... when != E = E1
+ when any
+ if (E == NULL) {
+...
+E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|
+ vzalloc_node\|vmalloc_exec\|vmalloc_32\|
+ vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|
+ __vmalloc_node\)(...)@vok
+...
+ }
+)
+
+@free@
+expression E;
+position fok;
+@@
+
+ E = \(kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|
+kvmalloc_array\)(...)
+ ...
+ kvfree(E)@fok
+
+@vfree depends on !patch@
+expression E;
+position a != alloc.kok;
+position f != free.fok;
+@@
+
+* E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|
+* kzalloc_node\|kmalloc_array\|kmalloc_array_node\|
+* kcalloc_node\)(...)@a
+ ... when != if (...) { ... E =
\(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\)(...);
... }
+ when != is_vmalloc_addr(E)
+ when any
+* \(vfree\|vfree_atomic\|kvfree\)(E)@f
+
+@depends on patch exists@
+expression E;
+position a != alloc.kok;
+position f != free.fok;
+@@
+
+ E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|
+kzalloc_node\|kmalloc_array\|kmalloc_array_node\|
+kcalloc_node\)(...)@a
+ ... when != if (...) { ... E =
\(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\)(...);
... }
+ when != is_vmalloc_addr(E)
+ when any
+- \(vfree\|vfree_atomic\|kvfree\)(E)@f
++ kfree(E)
+
+@kfree depends on !patch@
+expression E;
+position a != alloc.vok;
+position f != free.fok;
+@@
+
+* E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|
+* vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|
+* __vmalloc_node_range\|__vmalloc_node\)(...)@a
+ ... when != is_vmalloc_addr(E)
+ when any
+* \(kfree\|kfree_sensitive\|kvfree\)(E)@f
+
+@depends on patch exists@
+expression E;
+position a != alloc.vok;
+position f != free.fok;
+@@
+
+ E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|
+vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|
+__vmalloc_node_range\|__vmalloc_node\)(...)@a
+ ... when != is_vmalloc_addr(E)
+ when any
+- \(kfree\|kvfree\)(E)@f
++
[PATCH] coccinelle: api: kfree_sensitive: print memset position
Print memset() call position in addition to the kfree() position to ease issues identification. Signed-off-by: Denis Efremov --- scripts/coccinelle/api/kfree_sensitive.cocci | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/scripts/coccinelle/api/kfree_sensitive.cocci b/scripts/coccinelle/api/kfree_sensitive.cocci index e4a066a0b77d..8d980ebf3223 100644 --- a/scripts/coccinelle/api/kfree_sensitive.cocci +++ b/scripts/coccinelle/api/kfree_sensitive.cocci @@ -85,14 +85,16 @@ type T; @script:python depends on report@ p << r.p; +m << r.m; @@ -coccilib.report.print_report(p[0], - "WARNING: opportunity for kfree_sensitive/kvfree_sensitive") +msg = "WARNING opportunity for kfree_sensitive/kvfree_sensitive (memset at line %s)" +coccilib.report.print_report(p[0], msg % (m[0].line)) @script:python depends on org@ p << r.p; +m << r.m; @@ -coccilib.org.print_todo(p[0], - "WARNING: opportunity for kfree_sensitive/kvfree_sensitive") +msg = "WARNING opportunity for kfree_sensitive/kvfree_sensitive (memset at line %s)" +coccilib.org.print_todo(p[0], msg % (m[0].line)) -- 2.26.2
Re: kzfree script
On 10/2/20 5:13 PM, Julia Lawall wrote: > > > On Fri, 2 Oct 2020, Denis Efremov wrote: > >> Hi, >> >> On 10/2/20 5:01 PM, Julia Lawall wrote: >>> Denis, >>> >>> In the rule proposing kzfree_sensitive, I think it would be helpful to >>> also highlight the memset line. >> >> What do you mean? It's "highlighted" in context mode. Do you mean adding >> position argument to memset call and showing this position in the warning >> messages? > > Yes, that seems to be what I mean. 0-day generated a message from the > script, and I had to hunt around for the reason why it was doing that. So > it would be nice to have the memset highlighted. It seems that the > non-patch 0-day messages are generated from the report mode. > Ok, I will send a patch for it. Thanks, Denis
Re: kzfree script
Hi, On 10/2/20 5:01 PM, Julia Lawall wrote: > Denis, > > In the rule proposing kzfree_sensitive, I think it would be helpful to > also highlight the memset line. What do you mean? It's "highlighted" in context mode. Do you mean adding position argument to memset call and showing this position in the warning messages? Thanks, Denis
[PATCH v4] coccinelle: api: add kvmalloc script
Suggest kvmalloc, kvfree instead of opencoded patterns.
Signed-off-by: Denis Efremov
---
Changes in v2:
- binary operator cmp added
- NULL comparisions simplified
- "T x" case added to !patch mode
Changes in v3:
- kvfree rules added
Changes in v4:
- pattern updated to match only GFP_KERNEL/__GFP_NOWARN flags
to avoid possible false-positives
All patches are sent:
[1] https://lore.kernel.org/patchwork/patch/1296428/
[2] https://lore.kernel.org/patchwork/patch/1296636/
[3] https://lore.kernel.org/patchwork/patch/1282895/
[4] https://lore.kernel.org/patchwork/patch/1296631/
scripts/coccinelle/api/kvmalloc.cocci | 256 ++
1 file changed, 256 insertions(+)
create mode 100644 scripts/coccinelle/api/kvmalloc.cocci
diff --git a/scripts/coccinelle/api/kvmalloc.cocci
b/scripts/coccinelle/api/kvmalloc.cocci
new file mode 100644
index ..c30dab718a49
--- /dev/null
+++ b/scripts/coccinelle/api/kvmalloc.cocci
@@ -0,0 +1,256 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Find if/else condition with kmalloc/vmalloc calls.
+/// Suggest to use kvmalloc instead. Same for kvfree.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual patch
+virtual report
+virtual org
+virtual context
+
+@initialize:python@
+@@
+filter = frozenset(['kvfree'])
+
+def relevant(p):
+return not (filter & {el.current_element for el in p})
+
+@kvmalloc depends on !patch@
+expression E, E1, size;
+identifier flags;
+binary operator cmp = {<=, <, ==, >, >=};
+identifier x;
+type T;
+position p;
+@@
+
+(
+* if (size cmp E1 || ...)@p {
+...
+*E = \(kmalloc\|kzalloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+* kmalloc_array\|kmalloc_array_node\|kcalloc_node\)
+* (..., size,
\(flags\|GFP_KERNEL\|\(GFP_KERNEL\|flags\)|__GFP_NOWARN\), ...)
+...
+ } else {
+...
+*E = \(vmalloc\|vzalloc\|vmalloc_node\|vzalloc_node\)(..., size, ...)
+...
+ }
+|
+* E = \(kmalloc\|kzalloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+* kmalloc_array\|kmalloc_array_node\|kcalloc_node\)
+* (..., size, \(flags\|GFP_KERNEL\|\(GFP_KERNEL\|flags\)|__GFP_NOWARN\),
...)
+ ... when != E = E1
+ when != size = E1
+ when any
+* if (E == NULL)@p {
+...
+* E = \(vmalloc\|vzalloc\|vmalloc_node\|vzalloc_node\)(..., size, ...)
+...
+ }
+|
+* T x = \(kmalloc\|kzalloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+* kmalloc_array\|kmalloc_array_node\|kcalloc_node\)
+* (..., size,
\(flags\|GFP_KERNEL\|\(GFP_KERNEL\|flags\)|__GFP_NOWARN\), ...);
+ ... when != x = E1
+ when != size = E1
+ when any
+* if (x == NULL)@p {
+...
+* x = \(vmalloc\|vzalloc\|vmalloc_node\|vzalloc_node\)(..., size, ...)
+...
+ }
+)
+
+@kvfree depends on !patch@
+expression E;
+position p : script:python() { relevant(p) };
+@@
+
+* if (is_vmalloc_addr(E))@p {
+...
+* vfree(E)
+...
+ } else {
+... when != krealloc(E, ...)
+when any
+* \(kfree\|kzfree\)(E)
+...
+ }
+
+@depends on patch@
+expression E, E1, size, node;
+binary operator cmp = {<=, <, ==, >, >=};
+identifier flags, x;
+type T;
+@@
+
+(
+- if (size cmp E1)
+-E = kmalloc(size, flags);
+- else
+-E = vmalloc(size);
++ E = kvmalloc(size, flags);
+|
+- if (size cmp E1)
+-E = kmalloc(size, \(GFP_KERNEL\|GFP_KERNEL|__GFP_NOWARN\));
+- else
+-E = vmalloc(size);
++ E = kvmalloc(size, GFP_KERNEL);
+|
+- E = kmalloc(size, flags | __GFP_NOWARN);
+- if (E == NULL)
+- E = vmalloc(size);
++ E = kvmalloc(size, flags);
+|
+- E = kmalloc(size, \(GFP_KERNEL\|GFP_KERNEL|__GFP_NOWARN\));
+- if (E == NULL)
+- E = vmalloc(size);
++ E = kvmalloc(size, GFP_KERNEL);
+|
+- T x = kmalloc(size, flags | __GFP_NOWARN);
+- if (x == NULL)
+- x = vmalloc(size);
++ T x = kvmalloc(size, flags);
+|
+- T x = kmalloc(size, \(GFP_KERNEL\|GFP_KERNEL|__GFP_NOWARN\));
+- if (x == NULL)
+- x = vmalloc(size);
++ T x = kvmalloc(size, GFP_KERNEL);
+|
+- if (size cmp E1)
+-E = kzalloc(size, flags);
+- else
+-E = vzalloc(size);
++ E = kvzalloc(size, flags);
+|
+- if (size cmp E1)
+-E = kzalloc(size, \(GFP_KERNEL\|GFP_KERNEL|__GFP_NOWARN\));
+- else
+-E = vzalloc(size);
++ E = kvzalloc(size, GFP_KERNEL);
+|
+- E = kzalloc(size, flags | __GFP_NOWARN);
+- if (E == NULL)
+- E = vzalloc(size);
++ E = kvzalloc(size, flags);
+|
+- E = kzalloc(size, \(GFP_KERNEL\|GFP_KERNEL|__GFP_NOWARN\));
+- if (E == NULL)
+- E = vzalloc(size);
++ E = kvzalloc(size, GFP_KERNEL);
+|
+- T x = kzalloc(size, flags | __GFP_NOWARN);
+- if (x == NULL)
+- x = vzalloc(size);
++ T x = kvzalloc(size, flags);
+|
+- T x = kzalloc(size, \(GFP_KERNEL\|GFP_KERNEL|__GFP_NOWARN\));
+- if (x == NULL)
+- x = vzalloc(size);
++ T x = kvzalloc(size, GFP_KERNEL);
+|
+- if (size cmp E1)
+-E = kmalloc_node(size, flags, node);
+- else
+-E = vmalloc_node(size, node);
++ E = kvmalloc_node(si
[PATCH v3] coccinelle: misc: add flexible_array.cocci script
One-element and zero-length arrays are deprecated [1]. Kernel
code should always use "flexible array members" instead, except
for existing uapi definitions.
The script warns about one-element and zero-length arrays in structs.
[1] commit 68e4cd17e218 ("docs: deprecated.rst: Add zero-length and
one-element arrays")
Cc: Kees Cook
Cc: Gustavo A. R. Silva
Signed-off-by: Denis Efremov
---
Changes in v2:
- all uapi headers are now filtered-out. Unfortunately, coccinelle
doesn't provide structure names in Location.current_element.
For structures the field is always "something_else". Thus, there is
no easy way to create a list of existing structures in uapi headers
and suppress the warning only for them, but not for the newly added
uapi structures.
- The pattern doesn't require 2+ fields in a structure/union anymore.
Now it also checks single field structures/unions.
- The pattern simplified and now uses disjuction in array elements
(Thanks, Markus)
- Unions are removed from patch mode
- one-element arrays are removed from patch mode. Correct patch may
involve turning the array to a simple field instead of a flexible
array.
Changes in v3:
- exists removed from "depends on patch"
- position argument fixed in org mode
- link to the online documentation added to the warning message
scripts/coccinelle/misc/flexible_array.cocci | 88
1 file changed, 88 insertions(+)
create mode 100644 scripts/coccinelle/misc/flexible_array.cocci
diff --git a/scripts/coccinelle/misc/flexible_array.cocci
b/scripts/coccinelle/misc/flexible_array.cocci
new file mode 100644
index ..947fbaff82a9
--- /dev/null
+++ b/scripts/coccinelle/misc/flexible_array.cocci
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Zero-length and one-element arrays are deprecated, see
+/// Documentation/process/deprecated.rst
+/// Flexible-array members should be used instead.
+///
+//
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual context
+virtual report
+virtual org
+virtual patch
+
+@initialize:python@
+@@
+def relevant(positions):
+for p in positions:
+if "uapi" in p.file:
+ return False
+return True
+
+@r depends on !patch@
+identifier name, array;
+type T;
+position p : script:python() { relevant(p) };
+@@
+
+(
+ struct name {
+...
+* T array@p[\(0\|1\)];
+ };
+|
+ struct {
+...
+* T array@p[\(0\|1\)];
+ };
+|
+ union name {
+...
+* T array@p[\(0\|1\)];
+ };
+|
+ union {
+...
+* T array@p[\(0\|1\)];
+ };
+)
+
+@depends on patch@
+identifier name, array;
+type T;
+position p : script:python() { relevant(p) };
+@@
+
+(
+ struct name {
+...
+T array@p[
+- 0
+];
+ };
+|
+ struct {
+...
+T array@p[
+- 0
+];
+ };
+)
+
+@script: python depends on report@
+p << r.p;
+@@
+
+msg = "WARNING use flexible-array member instead
(https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays)"
+coccilib.report.print_report(p[0], msg)
+
+@script: python depends on org@
+p << r.p;
+@@
+
+msg = "WARNING use flexible-array member instead
(https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays)"
+coccilib.org.print_todo(p[0], msg)
--
2.26.2
Re: [PATCH v7] coccinelle: api: add kfree_mismatch script
Hi, On 8/3/20 9:34 PM, Denis Efremov wrote: > Check that alloc and free types of functions match each other. Julia, I've just send the patches to fix all the warnings emitted by the script. [1] https://lore.kernel.org/patchwork/patch/1309731/ [2] https://lore.kernel.org/patchwork/patch/1309273/ [3] https://lore.kernel.org/patchwork/patch/1309275/ Other inconsistencies and bugs detected by this script: 1e814d630fd1 drm/amd/display: Use kfree() to free rgb_user in calculate_user_regamma_ramp() 842540075974 drm/amd/display: Use kvfree() to free coeff in build_regamma() f5e383ac8b58 iommu/pamu: Use kzfree() in fsl_pamu_probe() 36b26e37 net/mlx5: Use kfree(ft->g) in arfs_create_groups() 114427b8927a drm/panfrost: Use kvfree() to free bo->sgts 742532d11d83 f2fs: use kfree() instead of kvfree() to free superblock data 47a357de2b6b net/mlx5: DR, Fix freeing in dr_create_rc_qp() a8c73c1a614f io_uring: use kvfree() in io_sqe_buffer_register() 7f89cc07d22a cxgb4: Use kfree() instead kvfree() where appropriate bb2359f4dbe9 bpf: Change kvfree to kfree in generic_map_lookup_batch() > Changes in v2: > - Lines are limited to 80 characters where possible > - Confidence changed from High to Medium because of >fs/btrfs/send.c:1119 false-positive > - __vmalloc_area_node() explicitly excluded from analysis >instead of !(file in "mm/vmalloc.c") condition > Changes in v3: > - prints style in org && report modes changed for python2 > Changes in v4: > - missing msg argument to print_todo fixed > Changes in v5: > - fix position p in kfree rule > - move @kok and @v positions in choice rule after the arguments > - remove kvmalloc suggestions > Changes in v6: > - more asterisks added in context mode > - second @kok added to the choice rule > Changes in v7: > - file renamed to kfree_mismatch.cocci > - python function relevant() removed > - additional rule for filtering free positions added > - btrfs false-positive fixed > - confidence level changed to high > - kvfree_switch rule added > - names for position variables changed to @a (alloc) and @f (free) Is there something I can improve in this cocci script to be accepted? Thanks, Denis
[PATCH 1/2] btrfs: use kvzalloc() to allocate clone_roots in btrfs_ioctl_send()
btrfs_ioctl_send() used open-coded kvzalloc implementation earlier.
The code was accidentally replaced with kzalloc() call [1]. Restore
the original code by using kvzalloc() to allocate sctx->clone_roots.
[1] https://patchwork.kernel.org/patch/9757891/#20529627
Cc: sta...@vger.kernel.org
Fixes: 818e010bf9d0 ("btrfs: replace opencoded kvzalloc with the helper")
Signed-off-by: Denis Efremov
---
fs/btrfs/send.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index d9813a5b075a..c874ddda6252 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -7181,7 +7181,7 @@ long btrfs_ioctl_send(struct file *mnt_file, struct
btrfs_ioctl_send_args *arg)
alloc_size = sizeof(struct clone_root) * (arg->clone_sources_count + 1);
- sctx->clone_roots = kzalloc(alloc_size, GFP_KERNEL);
+ sctx->clone_roots = kvzalloc(alloc_size, GFP_KERNEL);
if (!sctx->clone_roots) {
ret = -ENOMEM;
goto out;
--
2.26.2
[PATCH 2/2] btrfs: check allocation size in btrfs_ioctl_send()
Replace kvzalloc() call with kvcalloc() that checks
the size internally. Use array_size() helper to compute
the memory size for clone_sources_tmp.
Cc: Kees Cook
Signed-off-by: Denis Efremov
---
fs/btrfs/send.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index c874ddda6252..9e02aba30651 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -7087,7 +7087,7 @@ long btrfs_ioctl_send(struct file *mnt_file, struct
btrfs_ioctl_send_args *arg)
u32 i;
u64 *clone_sources_tmp = NULL;
int clone_sources_to_rollback = 0;
- unsigned alloc_size;
+ size_t alloc_size;
int sort_clone_roots = 0;
if (!capable(CAP_SYS_ADMIN))
@@ -7179,15 +7179,16 @@ long btrfs_ioctl_send(struct file *mnt_file, struct
btrfs_ioctl_send_args *arg)
sctx->waiting_dir_moves = RB_ROOT;
sctx->orphan_dirs = RB_ROOT;
- alloc_size = sizeof(struct clone_root) * (arg->clone_sources_count + 1);
-
- sctx->clone_roots = kvzalloc(alloc_size, GFP_KERNEL);
+ sctx->clone_roots = kvcalloc(sizeof(*sctx->clone_roots),
+arg->clone_sources_count + 1,
+GFP_KERNEL);
if (!sctx->clone_roots) {
ret = -ENOMEM;
goto out;
}
- alloc_size = arg->clone_sources_count * sizeof(*arg->clone_sources);
+ alloc_size = array_size(sizeof(*arg->clone_sources),
+ arg->clone_sources_count);
if (arg->clone_sources_count) {
clone_sources_tmp = kvmalloc(alloc_size, GFP_KERNEL);
--
2.26.2
[PATCH 2/2] net/mlx5e: Use kfree() to free fd->g in accel_fs_tcp_create_groups()
Memory ft->g in accel_fs_tcp_create_groups() is allocaed with kcalloc().
It's excessive to free ft->g with kvfree(). Use kfree() instead.
Signed-off-by: Denis Efremov
---
drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c
b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c
index 4cdd9eac647d..97f1594cee11 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c
@@ -191,7 +191,7 @@ static int accel_fs_tcp_create_groups(struct
mlx5e_flow_table *ft,
ft->g = kcalloc(MLX5E_ACCEL_FS_TCP_NUM_GROUPS, sizeof(*ft->g),
GFP_KERNEL);
in = kvzalloc(inlen, GFP_KERNEL);
if (!in || !ft->g) {
- kvfree(ft->g);
+ kfree(ft->g);
kvfree(in);
return -ENOMEM;
}
--
2.26.2
[PATCH 1/2] net/mlx5e: IPsec: Use kvfree() for memory allocated with kvzalloc()
Variables flow_group_in, spec in rx_fs_create() are allocated with
kvzalloc(). It's incorrect to free them with kfree(). Use kvfree()
instead.
Fixes: 5e466345291a ("net/mlx5e: IPsec: Add IPsec steering in local NIC RX")
Signed-off-by: Denis Efremov
---
drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
index 429428bbc903..b974f3cd1005 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
@@ -228,8 +228,8 @@ static int rx_fs_create(struct mlx5e_priv *priv,
fs_prot->miss_rule = miss_rule;
out:
- kfree(flow_group_in);
- kfree(spec);
+ kvfree(flow_group_in);
+ kvfree(spec);
return err;
}
--
2.26.2
[PATCH v2] coccinelle: misc: add excluded_middle.cocci script
Check for !A || A && B condition. It's equivalent to !A || B. Signed-off-by: Denis Efremov --- Changes in v2: - spelling mistake fixed - position variable moved on the && operator - patch pattern changed to - (A && B) - word "condition" removed from warning message scripts/coccinelle/misc/excluded_middle.cocci | 39 +++ 1 file changed, 39 insertions(+) create mode 100644 scripts/coccinelle/misc/excluded_middle.cocci diff --git a/scripts/coccinelle/misc/excluded_middle.cocci b/scripts/coccinelle/misc/excluded_middle.cocci new file mode 100644 index ..ab28393e4843 --- /dev/null +++ b/scripts/coccinelle/misc/excluded_middle.cocci @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0-only +/// +/// Condition !A || A && B is equivalent to !A || B. +/// +// Confidence: High +// Copyright: (C) 2020 Denis Efremov ISPRAS +// Options: --no-includes --include-headers + +virtual patch +virtual context +virtual org +virtual report + +@r depends on !patch@ +expression A, B; +position p; +@@ + +* !A || (A &&@p B) + +@depends on patch@ +expression A, B; +@@ + + !A || +- (A && B) ++ B + +@script:python depends on report@ +p << r.p; +@@ + +coccilib.report.print_report(p[0], "WARNING !A || A && B is equivalent to !A || B") + +@script:python depends on org@ +p << r.p; +@@ + +coccilib.org.print_todo(p[0], "WARNING !A || A && B is equivalent to !A || B") -- 2.26.2
Re: [PATCH v2] coccinelle: misc: add flexible_array.cocci script
Hi,
On 9/12/20 5:43 PM, Julia Lawall wrote:
>
>
> On Mon, 10 Aug 2020, Denis Efremov wrote:
>
>> Commit 68e4cd17e218 ("docs: deprecated.rst: Add zero-length and one-element
>> arrays") marks one-element and zero-length arrays as deprecated. Kernel
>> code should always use "flexible array members" instead.
>>
>> The script warns about one-element and zero-length arrays in structs.
>>
>> Cc: Kees Cook
>> Cc: Gustavo A. R. Silva
>> Signed-off-by: Denis Efremov
>> ---
>> Changes in v2:
>> - all uapi headers are now filtered-out. Unfortunately, coccinelle
>>doesn't provide structure names in Location.current_element.
>>For structures the field is always "something_else". Thus, there is
>>no easy way to create a list of existing structures in uapi headers
>>and suppress the warning only for them, but not for the newly added
>>uapi structures.
>> - The pattern doesn't require 2+ fields in a structure/union anymore.
>>Now it also checks single field structures/unions.
>> - The pattern simplified and now uses disjuction in array elements
>>(Thanks, Markus)
>> - Unions are removed from patch mode
>> - one-element arrays are removed from patch mode. Correct patch may
>>involve turning the array to a simple field instead of a flexible
>>array.
>>
>> On the current master branch, the rule generates:
>> - context: https://gist.github.com/evdenis/e2b4323491f9eff35376372df07f723c
>> - patch: https://gist.github.com/evdenis/46081da9d68ecefd07edc3769cebcf32
>>
>> scripts/coccinelle/misc/flexible_array.cocci | 88
>> 1 file changed, 88 insertions(+)
>> create mode 100644 scripts/coccinelle/misc/flexible_array.cocci
>>
>> diff --git a/scripts/coccinelle/misc/flexible_array.cocci
>> b/scripts/coccinelle/misc/flexible_array.cocci
>> new file mode 100644
>> index ..bf6dcda1783e
>> --- /dev/null
>> +++ b/scripts/coccinelle/misc/flexible_array.cocci
>> @@ -0,0 +1,88 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +///
>> +/// Zero-length and one-element arrays are deprecated, see
>> +/// Documentation/process/deprecated.rst
>> +/// Flexible-array members should be used instead.
>> +///
>> +//
>> +// Confidence: High
>> +// Copyright: (C) 2020 Denis Efremov ISPRAS.
>> +// Comments:
>> +// Options: --no-includes --include-headers
>> +
>> +virtual context
>> +virtual report
>> +virtual org
>> +virtual patch
>> +
>> +@initialize:python@
>> +@@
>> +def relevant(positions):
>> +for p in positions:
>> +if "uapi" in p.file:
>> + return False
>> +return True
>> +
>> +@r depends on !patch@
>> +identifier name, array;
>> +type T;
>> +position p : script:python() { relevant(p) };
>> +@@
>> +
>> +(
>> + struct name {
>> +...
>> +* T array@p[\(0\|1\)];
>> + };
>> +|
>> + struct {
>> +...
>> +* T array@p[\(0\|1\)];
>> + };
>> +|
>> + union name {
>> +...
>> +* T array@p[\(0\|1\)];
>> + };
>> +|
>> + union {
>> +...
>> +* T array@p[\(0\|1\)];
>> + };
>> +)
>> +
>> +@depends on patch exists@
>
> exists is not necessary here. There are not multiple control-flow paths
> through a structure declaration.
>
>> +identifier name, array;
>> +type T;
>> +position p : script:python() { relevant(p) };
>> +@@
>> +
>> +(
>> + struct name {
>> +...
>> +T array@p[
>> +- 0
>> +];
>> + };
>> +|
>> + struct {
>> +...
>> +T array@p[
>> +- 0
>> +];
>> + };
>> +)
>> +
>> +@script: python depends on report@
>> +p << r.p;
>> +@@
>> +
>> +msg = "WARNING: use flexible-array member instead"
>> +coccilib.report.print_report(p[0], msg)
>> +
>> +@script: python depends on org@
>> +p << r.p;
>> +@@
>> +
>> +msg = "WARNING: use flexible-array member instead"
>> +coccilib.org.print_todo(p, msg)
>
> This should be coccilib.org.print_todo(p[0], msg)
>
Thanks, I will send v3 with fixes and proper links to online documentation.
Regards,
Denis
Re: [PATCH] security: keys: Use kvfree_sensitive in a few places
Hi,
same patch
https://lkml.org/lkml/2020/8/27/168
Thanks,
Denis
On 9/11/20 2:44 PM, Alex Dewar wrote:
> In big_key.c, there are a few places where memzero_explicit + kvfree is
> used. It is better to use kvfree_sensitive instead, which is more
> readable and also prevents the compiler from eliding the call to
> memzero_explicit. Fix this.
>
> Signed-off-by: Alex Dewar
> ---
> security/keys/big_key.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/security/keys/big_key.c b/security/keys/big_key.c
> index 691347dea3c1..d17e5f09eeb8 100644
> --- a/security/keys/big_key.c
> +++ b/security/keys/big_key.c
> @@ -121,8 +121,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
> *path = file->f_path;
> path_get(path);
> fput(file);
> - memzero_explicit(buf, enclen);
> - kvfree(buf);
> + kvfree_sensitive(buf, enclen);
> } else {
> /* Just store the data in a buffer */
> void *data = kmalloc(datalen, GFP_KERNEL);
> @@ -140,8 +139,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
> err_enckey:
> kfree_sensitive(enckey);
> error:
> - memzero_explicit(buf, enclen);
> - kvfree(buf);
> + kvfree_sensitive(buf, enclen);
> return ret;
> }
>
> @@ -273,8 +271,7 @@ long big_key_read(const struct key *key, char *buffer,
> size_t buflen)
> err_fput:
> fput(file);
> error:
> - memzero_explicit(buf, enclen);
> - kvfree(buf);
> + kvfree_sensitive(buf, enclen);
> } else {
> ret = datalen;
> memcpy(buffer, key->payload.data[big_key_data], datalen);
>
Re: [PATCH] scripts: kzfree.cocci: Deprecate use of kzfree
Hi,
same patch
https://lkml.org/lkml/2020/8/11/130
Julia, I've send all the patches to fix existing
kfree_sensitive/kvfree_sensitive reports.
https://lkml.org/lkml/2020/8/27/168
https://lkml.org/lkml/2020/8/27/93
Thanks,
Denis
On 9/11/20 4:49 PM, Alex Dewar wrote:
> kzfree() is effectively deprecated as of commit 453431a54934 ("mm,
> treewide: rename kzfree() to kfree_sensitive()"). It is currently just a
> legacy alias for kfree_sensitive(), which achieves the same thing.
>
> Update kzfree.cocci accordingly:
> 1) Replace instances of kzfree with kfree_sensitive
> 2) Merge different rules for memset/memset_explicit as kzfree and
>kfree_sensitive are now equivalent
> 3) Rename script to kfree_sensitive.cocci
>
> In addition:
> 4) Move the script to the free/ subfolder, where it would seem to fit
>better
>
> Signed-off-by: Alex Dewar
> ---
> .../kfree_sensitive.cocci}| 38 +--
> 1 file changed, 10 insertions(+), 28 deletions(-)
> rename scripts/coccinelle/{api/kzfree.cocci => free/kfree_sensitive.cocci}
> (59%)
>
> diff --git a/scripts/coccinelle/api/kzfree.cocci
> b/scripts/coccinelle/free/kfree_sensitive.cocci
> similarity index 59%
> rename from scripts/coccinelle/api/kzfree.cocci
> rename to scripts/coccinelle/free/kfree_sensitive.cocci
> index 33625bd7cec9..a87f93f2ed5c 100644
> --- a/scripts/coccinelle/api/kzfree.cocci
> +++ b/scripts/coccinelle/free/kfree_sensitive.cocci
> @@ -1,13 +1,13 @@
> // SPDX-License-Identifier: GPL-2.0-only
> ///
> -/// Use kzfree, kvfree_sensitive rather than memset or
> -/// memzero_explicit followed by kfree
> +/// Use k{,v}free_sensitive rather than memset or memzero_explicit followed
> by
> +/// k{,v}free
> ///
> // Confidence: High
> // Copyright: (C) 2020 Denis Efremov ISPRAS
> // Options: --no-includes --include-headers
> //
> -// Keywords: kzfree, kvfree_sensitive
> +// Keywords: kfree_sensitive, kvfree_sensitive
> //
>
> virtual context
> @@ -18,7 +18,7 @@ virtual report
> @initialize:python@
> @@
> # kmalloc_oob_in_memset uses memset to explicitly trigger out-of-bounds
> access
> -filter = frozenset(['kmalloc_oob_in_memset', 'kzfree', 'kvfree_sensitive'])
> +filter = frozenset(['kmalloc_oob_in_memset', 'kfree_sensitive',
> 'kvfree_sensitive'])
>
> def relevant(p):
> return not (filter & {el.current_element for el in p})
> @@ -53,34 +53,16 @@ position m != cond.ok;
> type T;
> @@
>
> +(
> - memzero_explicit@m((T)E, size);
> - ... when != E
> - when strict
> -// TODO: uncomment when kfree_sensitive will be merged.
> -// Only this case is commented out because developers
> -// may not like patches like this since kzfree uses memset
> -// internally (not memzero_explicit).
> -//(
> -//- kfree(E)@p;
> -//+ kfree_sensitive(E);
> -//|
> -- \(vfree\|kvfree\)(E)@p;
> -+ kvfree_sensitive(E, size);
> -//)
> -
> -@rp_memset depends on patch@
> -expression E, size;
> -position p : script:python() { relevant(p) };
> -position m != cond.ok;
> -type T;
> -@@
> -
> +|
> - memset@m((T)E, 0, size);
> +)
>... when != E
>when strict
> (
> - kfree(E)@p;
> -+ kzfree(E);
> ++ kfree_sensitive(E);
> |
> - \(vfree\|kvfree\)(E)@p;
> + kvfree_sensitive(E, size);
> @@ -91,11 +73,11 @@ p << r.p;
> @@
>
> coccilib.report.print_report(p[0],
> - "WARNING: opportunity for kzfree/kvfree_sensitive")
> + "WARNING: opportunity for k{,v}free_sensitive")
>
> @script:python depends on org@
> p << r.p;
> @@
>
> coccilib.org.print_todo(p[0],
> - "WARNING: opportunity for kzfree/kvfree_sensitive")
> + "WARNING: opportunity for k{,v}free_sensitive")
>
[PATCH 1/2] ARM: makefile: Drop GZFLAGS definition and export
Drop the definition and export of GZFLAGS, because it's
not used. GZFLAGS was dropped from arm64 in commit
4cf234943dcf ("arm64: drop GZFLAGS definition and export").
Signed-off-by: Denis Efremov
---
arch/arm/Makefile | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 4e877354515f..3780063e0be0 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -20,7 +20,6 @@ ifeq ($(CONFIG_ARM_MODULE_PLTS),y)
KBUILD_LDS_MODULE += $(srctree)/arch/arm/kernel/module.lds
endif
-GZFLAGS:=-9
#KBUILD_CFLAGS +=-pipe
# Never generate .eh_frame
@@ -270,7 +269,7 @@ KBUILD_CPPFLAGS += $(patsubst
%,-I$(srctree)/%include,$(machdirs) $(platdirs))
endif
endif
-export TEXT_OFFSET GZFLAGS MMUEXT
+export TEXT_OFFSET MMUEXT
core-y += arch/arm/
# If we have a machine-specific directory, then include it in the build.
--
2.26.2
[PATCH 2/2] csky: Drop GZFLAGS definition
Drop the definition of GZFLAGS because it's not used. Signed-off-by: Denis Efremov --- arch/csky/Makefile | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/csky/Makefile b/arch/csky/Makefile index 37f593a4bf53..715b839bf998 100644 --- a/arch/csky/Makefile +++ b/arch/csky/Makefile @@ -1,6 +1,5 @@ # SPDX-License-Identifier: GPL-2.0-only OBJCOPYFLAGS :=-O binary -GZFLAGS:=-9 ifdef CONFIG_CPU_HAS_FPU FPUEXT = f -- 2.26.2
[PATCH 0/2] drop GZFLAGS definition
GZFLAGS is not used. KGZIP env var can be used to pass additional flags to gzip instead. Denis Efremov (2): ARM: makefile: Drop GZFLAGS definition and export csky: Drop GZFLAGS definition arch/arm/Makefile | 3 +-- arch/csky/Makefile | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) -- 2.26.2
Re: [PATCH 14/19] floppy: use a separate gendisk for each media format
Hi,
On 9/3/20 11:01 AM, Christoph Hellwig wrote:
> The floppy driver usually autodetects the media when used with the
> normal /dev/fd? devices, which also are the only nodes created by udev.
> But it also supports various aliases that force a given media format.
> That is currently supported using the blk_register_region framework
> which finds the floppy gendisk even for a 'mismatched' dev_t. The
> problem with this (besides the code complexity) is that it creates
> multiple struct block_device instances for the whole device of a
> single gendisk, which can lead to interesting issues in code not
> aware of that fact.
>
> To fix this just create a separate gendisk for each of the aliases
> if they are accessed.
>
> Signed-off-by: Christoph Hellwig
Tested-by: Denis Efremov
The patch looks ok as it is. Two nitpicks below if you will send next revision.
> ---
> drivers/block/floppy.c | 154 ++---
> 1 file changed, 97 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index a563b023458a8b..f07d97558cb698 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -402,7 +402,6 @@ static struct floppy_drive_params drive_params[N_DRIVE];
> static struct floppy_drive_struct drive_state[N_DRIVE];
> static struct floppy_write_errors write_errors[N_DRIVE];
> static struct timer_list motor_off_timer[N_DRIVE];
> -static struct gendisk *disks[N_DRIVE];
> static struct blk_mq_tag_set tag_sets[N_DRIVE];
> static struct block_device *opened_bdev[N_DRIVE];
> static DEFINE_MUTEX(open_lock);
> @@ -477,6 +476,8 @@ static struct floppy_struct floppy_type[32] = {
> { 3200,20,2,80,0,0x1C,0x00,0xCF,0x2C,"H1600" }, /* 31 1.6MB 3.5"*/
> };
>
> +static struct gendisk *disks[N_DRIVE][ARRAY_SIZE(floppy_type)];
> +
> #define SECTSIZE (_FD_SECTSIZE(*floppy))
>
> /* Auto-detection: Disk type used until the next media change occurs. */
> @@ -4109,7 +4110,7 @@ static int floppy_open(struct block_device *bdev,
> fmode_t mode)
>
> new_dev = MINOR(bdev->bd_dev);
> drive_state[drive].fd_device = new_dev;
> - set_capacity(disks[drive], floppy_sizes[new_dev]);
> + set_capacity(disks[drive][ITYPE(new_dev)], floppy_sizes[new_dev]);
> if (old_dev != -1 && old_dev != new_dev) {
> if (buffer_drive == drive)
> buffer_track = -1;
> @@ -4577,15 +4578,58 @@ static bool floppy_available(int drive)
> return true;
> }
>
> -static struct kobject *floppy_find(dev_t dev, int *part, void *data)
> +static int floppy_alloc_disk(unsigned int drive, unsigned int type)
> {
> - int drive = (*part & 3) | ((*part & 0x80) >> 5);
> - if (drive >= N_DRIVE || !floppy_available(drive))
> - return NULL;
> - if (((*part >> 2) & 0x1f) >= ARRAY_SIZE(floppy_type))
> - return NULL;
> - *part = 0;
> - return get_disk_and_module(disks[drive]);
> + struct gendisk *disk;
> + int err;
> +
> + disk = alloc_disk(1);
> + if (!disk)
> + return -ENOMEM;
> +
> + disk->queue = blk_mq_init_queue(&tag_sets[drive]);
> + if (IS_ERR(disk->queue)) {
> + err = PTR_ERR(disk->queue);
> + disk->queue = NULL;
> + put_disk(disk);
> + return err;
> + }
> +
> + blk_queue_bounce_limit(disk->queue, BLK_BOUNCE_HIGH);
> + blk_queue_max_hw_sectors(disk->queue, 64);
> + disk->major = FLOPPY_MAJOR;
> + disk->first_minor = TOMINOR(drive) | (type << 2);
> + disk->fops = &floppy_fops;
> + disk->events = DISK_EVENT_MEDIA_CHANGE;
> + if (type)
> + sprintf(disk->disk_name, "fd%d_type%d", drive, type);
> + else
> + sprintf(disk->disk_name, "fd%d", drive);
> + /* to be cleaned up... */
> + disk->private_data = (void *)(long)drive;
> + disk->flags |= GENHD_FL_REMOVABLE;
> +
> + disks[drive][type] = disk;
> + return 0;
> +}
> +
> +static DEFINE_MUTEX(floppy_probe_lock);
> +
> +static void floppy_probe(dev_t dev)
> +{
> + unsigned int drive = (MINOR(dev) & 3) | ((MINOR(dev) & 0x80) >> 5);
> + unsigned int type = (MINOR(dev) >> 2) & 0x1f;
ITYPE(MINOR(dev))?
> +
> + if (drive >= N_DRIVE || !floppy_available(drive) ||
> + type >= ARRAY_SIZE(floppy_type))
> + return;
> +
> + mutex_lock(&floppy_probe_lock);
> + if (!disks[drive][type]) {
> + if (floppy_alloc_
Re: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive()
Hi, On 9/2/20 4:10 PM, Van Leeuwen, Pascal wrote: >> -Original Message- >> From: linux-crypto-ow...@vger.kernel.org >> On Behalf Of Denis Efremov >> Sent: Thursday, August 27, 2020 8:44 AM >> To: linux-cry...@vger.kernel.org >> Cc: Denis Efremov ; Corentin Labbe >> ; Herbert Xu >> ; linux-kernel@vger.kernel.org >> Subject: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive() >> >> <<< External Email >>> >> Use kfree_sensitive() instead of open-coding it. >> >> Signed-off-by: Denis Efremov >> --- >> drivers/crypto/inside-secure/safexcel_hash.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/crypto/inside-secure/safexcel_hash.c >> b/drivers/crypto/inside-secure/safexcel_hash.c >> index 16a467969d8e..5ffdc1cd5847 100644 >> --- a/drivers/crypto/inside-secure/safexcel_hash.c >> +++ b/drivers/crypto/inside-secure/safexcel_hash.c >> @@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request >> *areq, >> } >> >> /* Avoid leaking */ >> -memzero_explicit(keydup, keylen); >> -kfree(keydup); >> +kfree_sensitive(keydup); >> > I'm not sure here ... I verified it does not break the driver (not a big > surprise), but ... > > memzero_explicit guarantees that it will not get optimized away and the > keydata _always_ > gets overwritten. Does kfree_sensitive also come with such a guarantee? I > could not find a > hard statement on that in its documentation. Although the "sensitive" part > surely suggests > it. kfree_sensitive() uses memzero_explicit() internally. > Additionally, this remark is made in the documentation for kfree_sensitive: > "this function > zeroes the whole allocated buffer which can be a good deal bigger than the > requested buffer > size passed to kmalloc(). So be careful when using this function in > performance sensitive > code" > > While the memzero_explicit does not zeroize anything beyond keylen. > Which is all you really need here, so why would you want to zeroize > potentially a lot more? > In any case the two are not fully equivalent. There are a number of predefined allocation sizes (power of 2) for faster alloc, i.e. https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L349 and it looks like that keys we free in this patches are in bounds of these sizes. As far as I understand, if a key is not a power of 2 len, the buffer will be zeroed to the closest power of 2 size. For small sizes like these, performance difference should be unnoticeable because of cache lines and how arch-optimized memzero() works. Key freeing doesn't look like a frequent event. Thanks, Denis
[PATCH] coccinelle: misc: add excluded_middle.cocci script
Check for "!A || A && B" condition. It's equivalent to "!A || B" condition. Signed-off-by: Denis Efremov --- scripts/coccinelle/misc/excluded_middle.cocci | 40 +++ 1 file changed, 40 insertions(+) create mode 100644 scripts/coccinelle/misc/excluded_middle.cocci diff --git a/scripts/coccinelle/misc/excluded_middle.cocci b/scripts/coccinelle/misc/excluded_middle.cocci new file mode 100644 index ..1b8c20f13966 --- /dev/null +++ b/scripts/coccinelle/misc/excluded_middle.cocci @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: GPL-2.0-only +/// +/// Condition "!A || A && B" is equalent to "!A || B". +/// +// Confidence: High +// Copyright: (C) 2020 Denis Efremov ISPRAS +// Options: --no-includes --include-headers + +virtual patch +virtual context +virtual org +virtual report + +@r depends on !patch@ +expression A, B; +position p; +@@ + +* !A || (A && B)@p + +@depends on patch@ +expression A, B; +@@ + + !A || +- (A && + B +- ) + +@script:python depends on report@ +p << r.p; +@@ + +coccilib.report.print_report(p[0], "WARNING condition !A || A && B is equivalent to !A || B") + +@script:python depends on org@ +p << r.p; +@@ + +coccilib.org.print_todo(p[0], "WARNING condition !A || A && B is equivalent to !A || B") -- 2.26.2
[PATCH] net: bcmgenet: fix mask check in bcmgenet_validate_flow()
VALIDATE_MASK(eth_mask->h_source) is checked twice in a row in
bcmgenet_validate_flow(). Add VALIDATE_MASK(eth_mask->h_dest)
instead.
Fixes: 3e370952287c ("net: bcmgenet: add support for ethtool rxnfc flows")
Cc: sta...@vger.kernel.org
Signed-off-by: Denis Efremov
---
I'm not sure that h_dest check is required here, it's only my guess.
Compile tested only.
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 0ca8436d2e9d..be85dad2e3bc 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1364,7 +1364,7 @@ static int bcmgenet_validate_flow(struct net_device *dev,
case ETHER_FLOW:
eth_mask = &cmd->fs.m_u.ether_spec;
/* don't allow mask which isn't valid */
- if (VALIDATE_MASK(eth_mask->h_source) ||
+ if (VALIDATE_MASK(eth_mask->h_dest) ||
VALIDATE_MASK(eth_mask->h_source) ||
VALIDATE_MASK(eth_mask->h_proto)) {
netdev_err(dev, "rxnfc: Unsupported mask\n");
--
2.26.2
Re: checkpatch? (was: Re: [PATCH v3] coccinelle: misc: add uninitialized_var.cocci script)
On 9/1/20 5:37 PM, Joe Perches wrote:
> On Tue, 2020-09-01 at 12:48 +0300, Denis Efremov wrote:
>> uninitialized_var() macro was removed from the sources [1] and
>> other warning-silencing tricks were deprecated [2]. The purpose of this
>> cocci script is to prevent new occurrences of uninitialized_var()
>> open-coded variants.
>
>> +(
>> +* T var =@p var;
>> +|
>> +* T var =@p *(&(var));
>> +|
>> +* var =@p var
>> +|
>> +* var =@p *(&(var))
>> +)
>
> Adding a checkpatch test might be a good thing too.
>
> ---
> scripts/checkpatch.pl | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 149518d2a6a7..300b2659aab3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3901,6 +3901,17 @@ sub process {
> #ignore lines not being added
> next if ($line =~ /^[^\+]/);
>
> +# check for self assigments used to avoid compiler warnings
> +# e.g.: int foo = foo, *bar = NULL;
> +#struct foo bar = *(&(bar));
> + if ($line =~ /^\+\s*(?:$Declare)?([A-Za-z_][A-Za-z\d_]*)\s*=/) {
> + my $var = $1;
> + if ($line =~
> /^\+\s*(?:$Declare)?$var\s*=\s*(?:$var|\*\s*\(?\s*&\s*\(?\s*$var\s*\)?\s*\)?)\s*[;,]/)
> {
> + WARN("SELF_ASSIGNMENT",
> + "Do not use self-assignments to avoid
> compiler warnings\n" . $herecurr);
> + }
> + }
> +
> # check for dereferences that span multiple lines
> if ($prevline =~ /^\+.*$Lval\s*(?:\.|->)\s*$/ &&
> $line =~ /^\+\s*(?!\#\s*(?!define\s+|if))\s*$Lval/) {
Looks good. I also faced this kind of assignments after declarations.
https://lkml.org/lkml/2020/8/31/85
I'm not sure if they are used to suppress compiler warnings, through.
Denis
[PATCH] coccinelle: ifnullfree: add vfree(), kvfree*() functions
Extend the list of free functions with kvfree(), kvfree_sensitive(), vfree(). Signed-off-by: Denis Efremov --- scripts/coccinelle/free/ifnullfree.cocci | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/scripts/coccinelle/free/ifnullfree.cocci b/scripts/coccinelle/free/ifnullfree.cocci index 2045391e36a0..285b92d5c665 100644 --- a/scripts/coccinelle/free/ifnullfree.cocci +++ b/scripts/coccinelle/free/ifnullfree.cocci @@ -20,8 +20,14 @@ expression E; - if (E != NULL) ( kfree(E); +| + kvfree(E); | kfree_sensitive(E); +| + kvfree_sensitive(E, ...); +| + vfree(E); | debugfs_remove(E); | @@ -42,9 +48,10 @@ position p; @@ * if (E != NULL) -* \(kfree@p\|kfree_sensitive@p\|debugfs_remove@p\|debugfs_remove_recursive@p\| +* \(kfree@p\|kvfree@p\|kfree_sensitive@p\|kvfree_sensitive@p\|vfree@p\| +* debugfs_remove@p\|debugfs_remove_recursive@p\| * usb_free_urb@p\|kmem_cache_destroy@p\|mempool_destroy@p\| -* dma_pool_destroy@p\)(E); +* dma_pool_destroy@p\)(E, ...); @script:python depends on org@ p << r.p; -- 2.26.2
[PATCH v3] coccinelle: misc: add uninitialized_var.cocci script
uninitialized_var() macro was removed from the sources [1] and
other warning-silencing tricks were deprecated [2]. The purpose of this
cocci script is to prevent new occurrences of uninitialized_var()
open-coded variants.
[1] commit 63a0895d960a ("compiler: Remove uninitialized_var() macro")
[2] commit 4b19bec97c88 ("docs: deprecated.rst: Add uninitialized_var()")
Cc: Kees Cook
Cc: Gustavo A. R. Silva
Signed-off-by: Denis Efremov
---
Changes in v2:
- Documentation cited in the script's description
- kernel.org link added to the diagnostics messages
- "T *var = &var;" pattern removed
- "var =@p var", "var =@p *(&(var))" patterns added
Changes in v3:
- commit's description changed
.../coccinelle/misc/uninitialized_var.cocci | 51 +++
1 file changed, 51 insertions(+)
create mode 100644 scripts/coccinelle/misc/uninitialized_var.cocci
diff --git a/scripts/coccinelle/misc/uninitialized_var.cocci
b/scripts/coccinelle/misc/uninitialized_var.cocci
new file mode 100644
index ..8fa845cefe11
--- /dev/null
+++ b/scripts/coccinelle/misc/uninitialized_var.cocci
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Please, don't reintroduce uninitialized_var().
+/// From Documentation/process/deprecated.rst:
+/// For any compiler warnings about uninitialized variables, just add
+/// an initializer. Using warning-silencing tricks is dangerous as it
+/// papers over real bugs (or can in the future), and suppresses unrelated
+/// compiler warnings (e.g. "unused variable"). If the compiler thinks it
+/// is uninitialized, either simply initialize the variable or make compiler
+/// changes. Keep in mind that in most cases, if an initialization is
+/// obviously redundant, the compiler's dead-store elimination pass will make
+/// sure there are no needless variable writes.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual context
+virtual report
+virtual org
+
+@r@
+identifier var;
+type T;
+position p;
+@@
+
+(
+* T var =@p var;
+|
+* T var =@p *(&(var));
+|
+* var =@p var
+|
+* var =@p *(&(var))
+)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0],
+ "WARNING this kind of initialization is deprecated
(https://www.kernel.org/doc/html/latest/process/deprecated.html#uninitialized-var)")
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0],
+ "WARNING this kind of initialization is deprecated
(https://www.kernel.org/doc/html/latest/process/deprecated.html#uninitialized-var)")
--
2.26.2
[PATCH] coccinelle: api: kobj_to_dev: don't warn about kobj_to_dev()
Exclude kobj_to_dev() definition from warnings.
Signed-off-by: Denis Efremov
---
No changes in performance. This patch can be squashed to the
original patch with kobj_to_dev.cocci script.
scripts/coccinelle/api/kobj_to_dev.cocci | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/scripts/coccinelle/api/kobj_to_dev.cocci
b/scripts/coccinelle/api/kobj_to_dev.cocci
index cd5d31c6fe76..d0b3b9647c19 100644
--- a/scripts/coccinelle/api/kobj_to_dev.cocci
+++ b/scripts/coccinelle/api/kobj_to_dev.cocci
@@ -15,10 +15,18 @@ virtual org
virtual patch
+@initialize:python@
+@@
+filter = frozenset(['kobj_to_dev'])
+
+def relevant(p):
+return not (filter & {el.current_element for el in p})
+
+
@r depends on !patch@
expression ptr;
symbol kobj;
-position p;
+position p : script:python() { relevant(p) };
@@
* container_of(ptr, struct device, kobj)@p
@@ -26,9 +34,10 @@ position p;
@depends on patch@
expression ptr;
+position p : script:python() { relevant(p) };
@@
-- container_of(ptr, struct device, kobj)
+- container_of(ptr, struct device, kobj)@p
+ kobj_to_dev(ptr)
--
2.26.2
[PATCH v2] coccinelle: misc: add uninitialized_var.cocci script
Commit 63a0895d960a ("compiler: Remove uninitialized_var() macro") and
commit 4b19bec97c88 ("docs: deprecated.rst: Add uninitialized_var()")
removed uninitialized_var() and deprecated it.
The purpose of this script is to prevent new occurrences of open-coded
variants of uninitialized_var().
Cc: Kees Cook
Cc: Gustavo A. R. Silva
Signed-off-by: Denis Efremov
---
Changes in v2:
- Documentation cited in the script's description
- kernel.org link added to the diagnostics messages
- "T *var = &var;" pattern removed
- "var =@p var", "var =@p *(&(var))" patterns added
.../coccinelle/misc/uninitialized_var.cocci | 51 +++
1 file changed, 51 insertions(+)
create mode 100644 scripts/coccinelle/misc/uninitialized_var.cocci
diff --git a/scripts/coccinelle/misc/uninitialized_var.cocci
b/scripts/coccinelle/misc/uninitialized_var.cocci
new file mode 100644
index ..8fa845cefe11
--- /dev/null
+++ b/scripts/coccinelle/misc/uninitialized_var.cocci
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Please, don't reintroduce uninitialized_var().
+/// From Documentation/process/deprecated.rst:
+/// For any compiler warnings about uninitialized variables, just add
+/// an initializer. Using warning-silencing tricks is dangerous as it
+/// papers over real bugs (or can in the future), and suppresses unrelated
+/// compiler warnings (e.g. "unused variable"). If the compiler thinks it
+/// is uninitialized, either simply initialize the variable or make compiler
+/// changes. Keep in mind that in most cases, if an initialization is
+/// obviously redundant, the compiler's dead-store elimination pass will make
+/// sure there are no needless variable writes.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual context
+virtual report
+virtual org
+
+@r@
+identifier var;
+type T;
+position p;
+@@
+
+(
+* T var =@p var;
+|
+* T var =@p *(&(var));
+|
+* var =@p var
+|
+* var =@p *(&(var))
+)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0],
+ "WARNING this kind of initialization is deprecated
(https://www.kernel.org/doc/html/latest/process/deprecated.html#uninitialized-var)")
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0],
+ "WARNING this kind of initialization is deprecated
(https://www.kernel.org/doc/html/latest/process/deprecated.html#uninitialized-var)")
--
2.26.2
[PATCH] kconfig: remove redundant assignment prompt = prompt
Semi-automatic removing of localization macros changed the line
from "prompt = _(prompt);" to "prompt = prompt;". Drop the
reduntand assignment.
Fixes: 694c49a7c01c ("kconfig: drop localization support")
Signed-off-by: Denis Efremov
---
scripts/kconfig/nconf.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
index daf1c1506ec4..e0f965529166 100644
--- a/scripts/kconfig/nconf.c
+++ b/scripts/kconfig/nconf.c
@@ -755,7 +755,6 @@ static void build_conf(struct menu *menu)
switch (ptype) {
case P_MENU:
child_count++;
- prompt = prompt;
if (single_menu_mode) {
item_make(menu, 'm',
"%s%*c%s",
--
2.26.2
Re: [PATCH V2] sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output
On 8/30/20 3:43 AM, Joe Perches wrote:
> $ cat sysfs_emit.cocci
> @@
> identifier d_show =~ "^.*show.*$";
I think this additional pattern will allow to take more functions into the
scope.
@da@
identifier show, store;
expression name, mode;
@@
(
DEVICE_ATTR(name, mode, show, store)
|
DEVICE_ATTR_PREALLOC(name, mode, show, store)
|
DEVICE_ATTR_IGNORE_LOCKDEP(name, mode, show, store)
)
@@
// I think device_show_ulong, device_show_int, device_show_bool
// functions deserve explicit handling because they are somewhat
// reference implementations.
identifier d_show = { da.show, device_show_ulong, device_show_int,
device_show_bool };
identifier dev, attr, buf;
@@
* ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
...
}
I tried also to handle DEVICE_ATTR_RW, but I failed to use fresh identifier.
This doesn't work:
@darw@
identifier name;
@@
(
DEVICE_ATTR_RW(name)
|
DEVICE_ATTR_RO(name)
|
DEVICE_ATTR_WO(name)
)
@@
identifier darw.name;
fresh identifier d_show = name ## "_show"; // <== parse error
identifier dev, attr, buf;
@@
* ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
...
}
Regards,
Denis
[PATCH] docs: filesystems: replace to_dev() with kobj_to_dev()
Commit a4232963757e ("driver-core: Move kobj_to_dev from genhd.h to device.h")
introduced kobj_to_dev() function.
Signed-off-by: Denis Efremov
---
Documentation/filesystems/sysfs.rst| 3 +--
Documentation/translations/zh_CN/filesystems/sysfs.txt | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/Documentation/filesystems/sysfs.rst
b/Documentation/filesystems/sysfs.rst
index ab0f7795792b..5a3209a4cebf 100644
--- a/Documentation/filesystems/sysfs.rst
+++ b/Documentation/filesystems/sysfs.rst
@@ -172,14 +172,13 @@ calls the associated methods.
To illustrate::
-#define to_dev(obj) container_of(obj, struct device, kobj)
#define to_dev_attr(_attr) container_of(_attr, struct device_attribute,
attr)
static ssize_t dev_attr_show(struct kobject *kobj, struct attribute *attr,
char *buf)
{
struct device_attribute *dev_attr = to_dev_attr(attr);
- struct device *dev = to_dev(kobj);
+ struct device *dev = kobj_to_dev(kobj);
ssize_t ret = -EIO;
if (dev_attr->show)
diff --git a/Documentation/translations/zh_CN/filesystems/sysfs.txt
b/Documentation/translations/zh_CN/filesystems/sysfs.txt
index 9481e3ed2a06..046cc1d52058 100644
--- a/Documentation/translations/zh_CN/filesystems/sysfs.txt
+++ b/Documentation/translations/zh_CN/filesystems/sysfs.txt
@@ -154,14 +154,13 @@ sysfs 会为这个类型调用适当的方法。当一个文件被读写时，
示例:
-#define to_dev(obj) container_of(obj, struct device, kobj)
#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
static ssize_t dev_attr_show(struct kobject *kobj, struct attribute *attr,
char *buf)
{
struct device_attribute *dev_attr = to_dev_attr(attr);
-struct device *dev = to_dev(kobj);
+struct device *dev = kobj_to_dev(kobj);
ssize_t ret = -EIO;
if (dev_attr->show)
--
2.26.2
[PATCH] Documentation: remove current_security() reference
Commit 15322a0d90b6 ("lsm: remove current_security()") removed
current_security() from the sources.
Signed-off-by: Denis Efremov
---
Documentation/security/credentials.rst | 1 -
1 file changed, 1 deletion(-)
diff --git a/Documentation/security/credentials.rst
b/Documentation/security/credentials.rst
index d9387209d143..357328d566c8 100644
--- a/Documentation/security/credentials.rst
+++ b/Documentation/security/credentials.rst
@@ -323,7 +323,6 @@ credentials (the value is simply returned in each case)::
uid_t current_fsuid(void) Current's file access UID
gid_t current_fsgid(void) Current's file access GID
kernel_cap_t current_cap(void) Current's effective capabilities
- void *current_security(void)Current's LSM security pointer
struct user_struct *current_user(void) Current's user account
There are also convenience wrappers for retrieving specific associated pairs of
--
2.26.2
Re: [PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE buffers.
> > Anyway, this will need updating, likely with better examples. > > diff --git a/Documentation/filesystems/sysfs.rst > b/Documentation/filesystems/sysfs.rst > index ab0f7795792b..13c7a86fa6c8 100644 > --- a/Documentation/filesystems/sysfs.rst > +++ b/Documentation/filesystems/sysfs.rst > @@ -242,12 +242,9 @@ Other notes: >is 4096. > > - show() methods should return the number of bytes printed into the > - buffer. This is the return value of scnprintf(). > + buffer. This is the return value of sysfs_emit(). > > -- show() must not use snprintf() when formatting the value to be > - returned to user space. If you can guarantee that an overflow > - will never happen you can use sprintf() otherwise you must use > - scnprintf(). > +- show() methods should only use sysfs_emit to format output. > I think it's good to reflect in docs that sysfs_emit_at/sysfs_emit_pos is only for "legacy" code and should not be used in new code (checkpatch.pl warning?) because of sysfs design principles. And something about newlines "General rule is to add newlines at the end of output." Thanks, Denis
Re: sysfs output without newlines
Hi, On 8/29/20 9:23 PM, Joe Perches wrote: > While doing an investigation for a possible treewide conversion of > sysfs output using sprintf/snprintf/scnprintf, I discovered > several instances of sysfs output without terminating newlines. > > It seems likely all of these should have newline terminations > or have the \n\r termination changed to a single newline. I think that it could break badly written scripts in rare cases. > > Anyone have any objection to patches adding newlines to these > in their original forms using sprintf/snprintf/scnprintf? I'm not sure about existing cases, but I think it's a good checkpatch.pl warning for new patches. It should be possible to check sysfs_emit() calls. Thanks, Denis
Re: [RFC PATCH] coccinelle: misc: add uninitialized_var.cocci script
On 8/29/20 10:48 PM, Julia Lawall wrote:
>
>
> On Sat, 29 Aug 2020, Joe Perches wrote:
>
>> On Sat, 2020-08-29 at 21:36 +0200, Julia Lawall wrote:
>>>
>>> On Wed, 12 Aug 2020, Denis Efremov wrote:
>>>
>>>> Commit 63a0895d960a ("compiler: Remove uninitialized_var() macro") and
>>>> commit 4b19bec97c88 ("docs: deprecated.rst: Add uninitialized_var()")
>>>> removed uninitialized_var() and deprecated it.
>>>>
>>>> The purpose of this script is to prevent new occurrences of open-coded
>>>> variants of uninitialized_var().
>>
>>>> Cc: Kees Cook
>>>> Cc: Gustavo A. R. Silva
>>>> Signed-off-by: Denis Efremov
>>>
>>> Applied, without the commented out part.
>>>
>>> I only got three warnings, though. Perhaps the others have been fixed?
>>
>> uninitialized_var does not exist in -next
Yes, and this rule checks for not introducing these initializations once again.
i.e, checks for:
int a = a;
int a = *(&a);
>
> OK, if it seems better, I can remove it. Out of the threee reported, one
> was a completely unnecessary initialization.
>
I would like send v2 with better description and link to the documentation
because it's
now available online:
https://www.kernel.org/doc/html/latest/process/deprecated.html#uninitialized-var
Thanks,
Denis
Re: [PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE buffers.
Hi,
On 8/29/20 1:52 AM, Joe Perches wrote:
> sprintf does not know the PAGE_SIZE maximum of the temporary buffer
> used for outputting sysfs content requests and it's possible to
> overrun the buffer length.
>
> Add a generic sysfs_emit mechanism that knows that the size of the
> temporary buffer and ensures that no overrun is done.
>
> Signed-off-by: Joe Perches
> ---
It could be a good idea to update the docs to, i.e.:
https://www.kernel.org/doc/html/latest/filesystems/sysfs.html
> fs/sysfs/file.c | 30 ++
> include/linux/sysfs.h | 8
> 2 files changed, 38 insertions(+)
>
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index eb6897ab78e7..06a13bbd7080 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -707,3 +707,33 @@ int sysfs_change_owner(struct kobject *kobj, kuid_t
> kuid, kgid_t kgid)
> return 0;
> }
> EXPORT_SYMBOL_GPL(sysfs_change_owner);
> +
> +/**
> + * sysfs_emit - scnprintf equivalent, aware of PAGE_SIZE buffer.
> + * @buf: start of PAGE_SIZE buffer.
> + * @pos: current position in buffer
> + * (pos - buf) must always be < PAGE_SIZE
> + * @fmt: format
> + * @...: arguments to format
> + *
> + *
> + * Returns number of characters written at pos.
> + */
> +int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
> +{
> + va_list args;
> + bool bad_pos = pos < buf;
> + bool bad_len = (pos - buf) >= PAGE_SIZE;
> + int len;
> +
> + if (WARN(bad_pos || bad_len, "(pos < buf):%d (pos >= PAGE_SIZE):%d\n",
> + bad_pos, bad_len))
> + return 0;
> +
> + va_start(args, fmt);
> + len = vscnprintf(pos, PAGE_SIZE - (pos - buf), fmt, args);
> + va_end(args);
> +
> + return len;
> +}
> +EXPORT_SYMBOL_GPL(sysfs_emit);
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 34e84122f635..5a21d3d30016 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -329,6 +329,8 @@ int sysfs_groups_change_owner(struct kobject *kobj,
> int sysfs_group_change_owner(struct kobject *kobj,
>const struct attribute_group *groups, kuid_t kuid,
>kgid_t kgid);
> +__printf(3, 4)
> +int sysfs_emit(char *buf, char *pos, const char *fmt, ...);
>
> #else /* CONFIG_SYSFS */
>
> @@ -576,6 +578,12 @@ static inline int sysfs_group_change_owner(struct
> kobject *kobj,
> return 0;
> }
>
> +__printf(3, 4)
> +static inline int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
> +{
> + return 0;
> +}
> +
> #endif /* CONFIG_SYSFS */
>
> static inline int __must_check sysfs_create_file(struct kobject *kobj,
>
Thanks,
Denis
[RFC PATCH] coccinelle: api: add flex_array_size.cocci script
Suggest flex_array_size() wrapper to compute the size of a
flexible array member in a structure. The macro additionally
checks for integer overflows.
The cocci script intentionally skips cases where count argument
is not a member of a structure because this introduce false
positives.
Cc: Gustavo A. R. Silva
Cc: Kees Cook
Signed-off-by: Denis Efremov
---
Kees, Gustavo, may I have your acks if you find this script useful?
Currently, it emits following warnings:
./fs/select.c:994:25-26: WARNING opportunity for flex_array_size
./include/linux/avf/virtchnl.h:711:34-35: WARNING opportunity for
flex_array_size
./include/linux/avf/virtchnl.h:722:43-44: WARNING opportunity for
flex_array_size
./include/linux/avf/virtchnl.h:738:40-41: WARNING opportunity for
flex_array_size
./include/linux/avf/virtchnl.h:749:46-47: WARNING opportunity for
flex_array_size
./drivers/dma/qcom/bam_dma.c:1055:35-36: WARNING opportunity for flex_array_size
./drivers/md/dm-crypt.c:2895:45-46: WARNING opportunity for flex_array_size
./drivers/md/dm-crypt.c:3381:47-48: WARNING opportunity for flex_array_size
./drivers/md/dm-crypt.c:2484:45-46: WARNING opportunity for flex_array_size
./drivers/md/dm-crypt.c:2484:45-46: WARNING opportunity for flex_array_size
./net/sched/em_canid.c:198:48-49: WARNING opportunity for flex_array_size
./include/linux/filter.h:741:42-43: WARNING opportunity for flex_array_size
./fs/aio.c:677:42-43: WARNING opportunity for flex_array_size
./include/rdma/rdmavt_qp.h:537:31-32: WARNING opportunity for flex_array_size
./include/rdma/rdmavt_qp.h:537:31-32: WARNING opportunity for flex_array_size
./lib/ts_fsm.c:311:49-50: WARNING opportunity for flex_array_size
./mm/slab.c:3407:59-60: WARNING opportunity for flex_array_size
./mm/slab.c:2139:55-56: WARNING opportunity for flex_array_size
./mm/slab.c:3407:59-60: WARNING opportunity for flex_array_size
./mm/slab.c:2139:55-56: WARNING opportunity for flex_array_size
scripts/coccinelle/api/flex_array_size.cocci | 180 +++
1 file changed, 180 insertions(+)
create mode 100644 scripts/coccinelle/api/flex_array_size.cocci
diff --git a/scripts/coccinelle/api/flex_array_size.cocci
b/scripts/coccinelle/api/flex_array_size.cocci
new file mode 100644
index ..b5264a826c29
--- /dev/null
+++ b/scripts/coccinelle/api/flex_array_size.cocci
@@ -0,0 +1,180 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Suggest flex_array_size() wrapper to compute the size of a
+/// flexible array member in a structure. The macro additionally
+/// checks for integer overflows.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: flex_array_size
+//
+
+
+virtual context
+virtual report
+virtual org
+virtual patch
+
+@decl_flex@
+identifier name, array, size;
+type TA, TS;
+@@
+
+ struct name {
+...
+TS size;
+...
+(
+TA array[];
+|
+TA array[\(0\|1\)];
+)
+ };
+
+@ptr_flex@
+identifier decl_flex.name;
+identifier instance;
+@@
+
+ struct name *instance;
+
+@struct_flex@
+identifier decl_flex.name;
+identifier instance;
+@@
+
+ struct name instance;
+
+@ptr_flex_size depends on !patch@
+identifier decl_flex.array, decl_flex.size;
+identifier ptr_flex.instance;
+type decl_flex.TA;
+position p;
+@@
+
+(
+* instance->size * sizeof(TA)@p
+|
+* instance->size * sizeof(*instance->array)@p
+)
+
+@depends on patch exists@
+identifier decl_flex.array, decl_flex.size;
+identifier ptr_flex.instance;
+type decl_flex.TA;
+@@
+
+(
+- instance->size * sizeof(TA)
++ flex_array_size(instance, array, instance->size)
+|
+- instance->size * sizeof(*instance->array)
++ flex_array_size(instance, array, instance->size)
+)
+
+@struct_flex_size depends on !patch@
+identifier decl_flex.array, decl_flex.size;
+identifier struct_flex.instance;
+type decl_flex.TA;
+position p;
+@@
+
+(
+* instance.size * sizeof(TA)@p
+|
+* instance.size * sizeof(*instance->array)@p
+)
+
+@depends on patch exists@
+identifier decl_flex.array, decl_flex.size;
+identifier struct_flex.instance;
+type decl_flex.TA;
+@@
+
+(
+- instance.size * sizeof(TA)
++ flex_array_size(instance, array, instance.size)
+|
+- instance.size * sizeof(*instance->array)
++ flex_array_size(instance, array, instance.size)
+)
+
+@func_arg_flex_size depends on !patch@
+identifier decl_flex.name, decl_flex.array, decl_flex.size;
+identifier func, instance;
+type decl_flex.TA;
+position p;
+@@
+
+ func(..., struct name *instance, ...) {
+... when any
+(
+* instance->size * sizeof(TA)@p
+|
+* instance->size * sizeof(*instance->array)@p
+)
+...
+ }
+
+@depends on patch exists@
+identifier decl_flex.name, decl_flex.array, decl_flex.size;
+identifier func, instance;
+type decl_flex.TA;
+@@
+
+ func(..., struct name *instance, ...) {
+... when any
+(
+- instance->size * sizeof(TA)
++ flex_array_size(instance, array, instance->size)
+|
+- instance->size * sizeof
Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs
>
> I tried:
> @@
> identifier f_show =~ "^.*_show$";
This will miss this kind of functions:
./drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1953:static
DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
./drivers/gpu/drm/amd/amdgpu/df_v3_6.c:266:static DEVICE_ATTR(df_cntr_avail,
S_IRUGO, df_v3_6_get_df_cntr_avail, NULL);
./drivers/input/touchscreen/melfas_mip4.c:1348:static DEVICE_ATTR(fw_version,
S_IRUGO, mip4_sysfs_read_fw_version, NULL);
./drivers/input/touchscreen/melfas_mip4.c:1373:static DEVICE_ATTR(hw_version,
S_IRUGO, mip4_sysfs_read_hw_version, NULL);
./drivers/input/touchscreen/melfas_mip4.c:1392:static DEVICE_ATTR(product_id,
S_IRUGO, mip4_sysfs_read_product_id, NULL);
...
> identifier dev, attr, buf;
> const char *chr;
> @@
> ssize_t f_show(struct device *dev, struct device_attribute *attr, char
> *buf)
> {
> <...
> (
> - sprintf
> + sysfs_sprintf
> (...);
> |
> - snprintf(buf, PAGE_SIZE,
> + sysfs_sprintf(buf,
> ...);
> |
> - scnprintf(buf, PAGE_SIZE,
> + sysfs_sprintf(buf,
> ...);
> |
> strcpy(buf, chr);
> sysfs_strcpy(buf, chr);
> )
> ...>
> }
>
> which finds direct statements without an assign
> but that doesn't find
>
> arch/arm/common/dmabounce.c:static ssize_t dmabounce_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> arch/arm/common/dmabounce.c-{
> arch/arm/common/dmabounce.c-struct dmabounce_device_info *device_info =
> dev->archdata.dmabounce;
> arch/arm/common/dmabounce.c-return sprintf(buf, "%lu %lu %lu %lu %lu
> %lu\n",
> arch/arm/common/dmabounce.c-device_info->small.allocs,
> arch/arm/common/dmabounce.c-device_info->large.allocs,
> arch/arm/common/dmabounce.c-device_info->total_allocs -
> device_info->small.allocs -
> arch/arm/common/dmabounce.c-device_info->large.allocs,
> arch/arm/common/dmabounce.c-device_info->total_allocs,
> arch/arm/common/dmabounce.c-device_info->map_op_count,
> arch/arm/common/dmabounce.c-device_info->bounce_count);
> arch/arm/common/dmabounce.c-}
>
This will match it (the difference is in the ';'):
@@
identifier f_show =~ "^.*_show$";
identifier dev, attr, buf;
@@
ssize_t f_show(struct device *dev, struct device_attribute *attr, char *buf)
{
<...
- sprintf
+ sysfs_sprintf
(...)
...>
}
Regards,
Denis
[PATCH v3] udf: Use kvzalloc() in udf_sb_alloc_bitmap()
Use kvzalloc() in udf_sb_alloc_bitmap() instead of open-coding it.
Size computation wrapped in struct_size() macro to prevent potential
integer overflows.
Signed-off-by: Denis Efremov
---
Changes in v2:
- size computation wrapped in struct_size()
Changes in v3:
- int size dropped
fs/udf/super.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 1c42f544096d..d9eabbe368ff 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -1006,18 +1006,10 @@ int udf_compute_nr_groups(struct super_block *sb, u32
partition)
static struct udf_bitmap *udf_sb_alloc_bitmap(struct super_block *sb, u32
index)
{
struct udf_bitmap *bitmap;
- int nr_groups;
- int size;
-
- nr_groups = udf_compute_nr_groups(sb, index);
- size = sizeof(struct udf_bitmap) +
- (sizeof(struct buffer_head *) * nr_groups);
-
- if (size <= PAGE_SIZE)
- bitmap = kzalloc(size, GFP_KERNEL);
- else
- bitmap = vzalloc(size); /* TODO: get rid of vzalloc */
+ int nr_groups = udf_compute_nr_groups(sb, index);
+ bitmap = kvzalloc(struct_size(bitmap, s_block_bitmap, nr_groups),
+ GFP_KERNEL);
if (!bitmap)
return NULL;
--
2.26.2
Re: [PATCH v2] udf: Use kvzalloc() in udf_sb_alloc_bitmap()
On 8/28/20 1:09 AM, Gustavo A. R. Silva wrote: > > > On 8/27/20 16:25, Denis Efremov wrote: >> Use kvzalloc() in udf_sb_alloc_bitmap() instead of open-coding it. >> Size computation wrapped in struct_size() macro to prevent potential >> integer overflows. >> >> Signed-off-by: Denis Efremov >> --- > > Please, comment here what changed in v2, vn... e.g.: > > Changes in v2: > - Use struct_size() helper. > Ah, thanks. I added this initially and accidentally regenerated the patch file with format-patch. > > Why not this: > > bitmap = kvzalloc(struct_size(bitmap, s_block_bitmap, nr_groups), > GFP_KERNEL); > > and you can also get rid of _size_ entirely. > My bad, I missed that only nr_groups is used down the code. Thanks, I will resend it as v3. Denis
[PATCH] char: mspec: Use kvzalloc() in mspec_mmap()
Use kvzalloc() in mspec_mmap() instead of open-coding it. Signed-off-by: Denis Efremov --- drivers/char/mspec.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/char/mspec.c b/drivers/char/mspec.c index 0fae33319d2e..f8231e2e84be 100644 --- a/drivers/char/mspec.c +++ b/drivers/char/mspec.c @@ -195,10 +195,7 @@ mspec_mmap(struct file *file, struct vm_area_struct *vma, pages = vma_pages(vma); vdata_size = sizeof(struct vma_data) + pages * sizeof(long); - if (vdata_size <= PAGE_SIZE) - vdata = kzalloc(vdata_size, GFP_KERNEL); - else - vdata = vzalloc(vdata_size); + vdata = kvzalloc(vdata_size, GFP_KERNEL); if (!vdata) return -ENOMEM; -- 2.26.2
[PATCH v2] udf: Use kvzalloc() in udf_sb_alloc_bitmap()
Use kvzalloc() in udf_sb_alloc_bitmap() instead of open-coding it. Size computation wrapped in struct_size() macro to prevent potential integer overflows. Signed-off-by: Denis Efremov --- fs/udf/super.c | 9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/fs/udf/super.c b/fs/udf/super.c index 1c42f544096d..bdf51bea54f3 100644 --- a/fs/udf/super.c +++ b/fs/udf/super.c @@ -1010,14 +1010,9 @@ static struct udf_bitmap *udf_sb_alloc_bitmap(struct super_block *sb, u32 index) int size; nr_groups = udf_compute_nr_groups(sb, index); - size = sizeof(struct udf_bitmap) + - (sizeof(struct buffer_head *) * nr_groups); - - if (size <= PAGE_SIZE) - bitmap = kzalloc(size, GFP_KERNEL); - else - bitmap = vzalloc(size); /* TODO: get rid of vzalloc */ + size = struct_size(bitmap, s_block_bitmap, nr_groups); + bitmap = kvzalloc(size, GFP_KERNEL); if (!bitmap) return NULL; -- 2.26.2
Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs
Hi all,
On 8/27/20 10:42 PM, Julia Lawall wrote:
>
>
> On Thu, 27 Aug 2020, Joe Perches wrote:
>
>> On Thu, 2020-08-27 at 15:48 +0100, Alex Dewar wrote:
>>> On Thu, Aug 27, 2020 at 03:41:06PM +0200, Rasmus Villemoes wrote:
On 27/08/2020 15.18, Alex Dewar wrote:
> On Thu, Aug 27, 2020 at 09:15:37AM +0200, Greg Kroah-Hartman wrote:
>> On Thu, Aug 27, 2020 at 08:42:06AM +0200, Rasmus Villemoes wrote:
>>> On 25/08/2020 00.23, Alex Dewar wrote:
kernel/cpu.c: don't use snprintf() for sysfs attrs
As per the documentation (Documentation/filesystems/sysfs.rst),
snprintf() should not be used for formatting values returned by sysfs.
Just FYI, I've send an addition to the device_attr_show.cocci script[1] to turn
simple cases of snprintf (e.g. "%i") to sprintf. Looks like many developers
would
like it more than changing snprintf to scnprintf. As for me, I don't like the
idea
of automated altering of the original logic from bounded snprint to unbouded one
with sprintf.
[1] https://lkml.org/lkml/2020/8/13/786
Regarding current device_attr_show.cocci implementation, it detects the
functions
by declaration:
ssize_t any_name(struct device *dev, struct device_attribute *attr, char *buf)
and I limited the check to:
"return snprintf"
pattern because there are already too many warnings.
Actually, it looks more correct to check for:
ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
{
<...
* snprintf@p(...);
...>
}
This pattern should also highlight the snprintf calls there we save returned
value in a var, e.g.:
ret += snprintf(...);
...
ret += snprintf(...);
...
ret += snprintf(...);
return ret;
>
> Something like
>
> identifier f;
> fresh identifier = "sysfs" ## f;
>
> may be useful. Let me know if further help is needed.
Initially, I wrote the rule to search for DEVICE_ATTR(..., ..., func_name, ...)
functions. However, it looks like matching function prototype is enough. At
least,
I failed to find false positives. I rejected the initial DEVICE_ATTR() searching
because I thought that it's impossible to handle
DEVICE_ATTR_RO()/DEVICE_ATTR_RW()
macroses with coccinelle as they "generate" function names internally with
"##". "fresh identifier" should really help here, but now I doubt it's required
in
device_attr_show.cocci, function prototype is enough.
Thanks,
Denis
[PATCH v2] ARM/dma-mapping: use kvzalloc() in __iommu_alloc_buffer()
Use kvzalloc() in __iommu_alloc_buffer() instead of open-coding it.
Size computation wrapped in array_size() macro to prevent potential
integer overflows.
Signed-off-by: Denis Efremov
---
Changes in v2:
- array_size() added
arch/arm/mm/dma-mapping.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 8a8949174b1c..c1f864ff7b84 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1199,14 +1199,10 @@ static struct page **__iommu_alloc_buffer(struct device
*dev, size_t size,
{
struct page **pages;
int count = size >> PAGE_SHIFT;
- int array_size = count * sizeof(struct page *);
int i = 0;
int order_idx = 0;
- if (array_size <= PAGE_SIZE)
- pages = kzalloc(array_size, GFP_KERNEL);
- else
- pages = vzalloc(array_size);
+ pages = kvzalloc(array_size(sizeof(*pages), count), GFP_KERNEL);
if (!pages)
return NULL;
--
2.26.2
Re: [PATCH] udf: Use kvzalloc() in udf_sb_alloc_bitmap()
> @@ -1013,10 +1013,7 @@ static struct udf_bitmap *udf_sb_alloc_bitmap(struct > super_block *sb, u32 index) > size = sizeof(struct udf_bitmap) + > (sizeof(struct buffer_head *) * nr_groups); I missed that this size is a good place to use struct_size for overflow checking. I will send v2 instead.
[PATCH] udf: Use kvzalloc() in udf_sb_alloc_bitmap()
Use kvzalloc() in udf_sb_alloc_bitmap() instead of open-coding it. Signed-off-by: Denis Efremov --- I'm not sure about TODO comment, through. fs/udf/super.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/fs/udf/super.c b/fs/udf/super.c index 1c42f544096d..c7cd15219b7c 100644 --- a/fs/udf/super.c +++ b/fs/udf/super.c @@ -1013,10 +1013,7 @@ static struct udf_bitmap *udf_sb_alloc_bitmap(struct super_block *sb, u32 index) size = sizeof(struct udf_bitmap) + (sizeof(struct buffer_head *) * nr_groups); - if (size <= PAGE_SIZE) - bitmap = kzalloc(size, GFP_KERNEL); - else - bitmap = vzalloc(size); /* TODO: get rid of vzalloc */ + bitmap = kvzalloc(size, GFP_KERNEL); if (!bitmap) return NULL; -- 2.26.2
[PATCH] ARM/dma-mapping: use kvzalloc() in __iommu_alloc_buffer()
Use kvzalloc() in __iommu_alloc_buffer() instead of open-coding it. Signed-off-by: Denis Efremov --- arch/arm/mm/dma-mapping.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 8a8949174b1c..9def10affa70 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1203,10 +1203,7 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size, int i = 0; int order_idx = 0; - if (array_size <= PAGE_SIZE) - pages = kzalloc(array_size, GFP_KERNEL); - else - pages = vzalloc(array_size); + pages = kvzalloc(array_size, GFP_KERNEL); if (!pages) return NULL; -- 2.26.2
Re: [PATCH v2] scsi: libcxgbi: use kvzalloc instead of opencoded kzalloc/vzalloc
Ping?
On 8/1/20 4:31 PM, Denis Efremov wrote:
> Remove cxgbi_alloc_big_mem(), cxgbi_free_big_mem() functions
> and use kvzalloc/kvfree instead. __GFP_NOWARN added to kvzalloc()
> call because we already print a warning in case of allocation fail.
>
> Signed-off-by: Denis Efremov
> ---
> drivers/scsi/cxgbi/libcxgbi.c | 8
> drivers/scsi/cxgbi/libcxgbi.h | 16
> 2 files changed, 4 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
> index 4bc794d2f51c..51f4d34da73f 100644
> --- a/drivers/scsi/cxgbi/libcxgbi.c
> +++ b/drivers/scsi/cxgbi/libcxgbi.c
> @@ -77,9 +77,9 @@ int cxgbi_device_portmap_create(struct cxgbi_device *cdev,
> unsigned int base,
> {
> struct cxgbi_ports_map *pmap = &cdev->pmap;
>
> - pmap->port_csk = cxgbi_alloc_big_mem(max_conn *
> - sizeof(struct cxgbi_sock *),
> - GFP_KERNEL);
> + pmap->port_csk = kvzalloc(array_size(max_conn,
> + sizeof(struct cxgbi_sock *)),
> + GFP_KERNEL | __GFP_NOWARN);
> if (!pmap->port_csk) {
> pr_warn("cdev 0x%p, portmap OOM %u.\n", cdev, max_conn);
> return -ENOMEM;
> @@ -124,7 +124,7 @@ static inline void cxgbi_device_destroy(struct
> cxgbi_device *cdev)
> if (cdev->cdev2ppm)
> cxgbi_ppm_release(cdev->cdev2ppm(cdev));
> if (cdev->pmap.max_connect)
> - cxgbi_free_big_mem(cdev->pmap.port_csk);
> + kvfree(cdev->pmap.port_csk);
> kfree(cdev);
> }
>
> diff --git a/drivers/scsi/cxgbi/libcxgbi.h b/drivers/scsi/cxgbi/libcxgbi.h
> index 84b96af52655..321426242be4 100644
> --- a/drivers/scsi/cxgbi/libcxgbi.h
> +++ b/drivers/scsi/cxgbi/libcxgbi.h
> @@ -537,22 +537,6 @@ struct cxgbi_task_data {
> #define iscsi_task_cxgbi_data(task) \
> ((task)->dd_data + sizeof(struct iscsi_tcp_task))
>
> -static inline void *cxgbi_alloc_big_mem(unsigned int size,
> - gfp_t gfp)
> -{
> - void *p = kzalloc(size, gfp | __GFP_NOWARN);
> -
> - if (!p)
> - p = vzalloc(size);
> -
> - return p;
> -}
> -
> -static inline void cxgbi_free_big_mem(void *addr)
> -{
> - kvfree(addr);
> -}
> -
> static inline void cxgbi_set_iscsi_ipv4(struct cxgbi_hba *chba, __be32
> ipaddr)
> {
> if (chba->cdev->flags & CXGBI_FLAG_IPV4_SET)
>
[PATCH v2] crypto: sun8i-ss - remove redundant memzero_explicit()
Remove redundant memzero_explicit() in sun8i_ss_cipher() before calling
kfree_sensitive(). kfree_sensitive() will zero the memory with
memzero_explicit().
Fixes: 453431a54934 ("mm, treewide: rename kzfree() to kfree_sensitive()")
Signed-off-by: Denis Efremov
---
Changes in v2:
- fixes tag added
drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
index deb8b39a86db..ed2a69f82e1c 100644
--- a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
@@ -248,7 +248,6 @@ static int sun8i_ss_cipher(struct skcipher_request *areq)
offset = areq->cryptlen - ivsize;
if (rctx->op_dir & SS_DECRYPTION) {
memcpy(areq->iv, backup_iv, ivsize);
- memzero_explicit(backup_iv, ivsize);
kfree_sensitive(backup_iv);
} else {
scatterwalk_map_and_copy(areq->iv, areq->dst,
offset,
--
2.26.2
Re: [PATCH] crypto: sun8i-ss - remove redundant memzero_explicit()
On 8/27/20 11:03 AM, Corentin Labbe wrote:
> Could you add:
> Fixes: 453431a54934 ("mm, treewide: rename kzfree() to kfree_sensitive()")
I doubt this change deserves fixes tag, since this is just a cleanup.
Anyway, I will send v2 with it.
Thanks,
Denis
[PATCH] crypto: sun8i-ss - remove redundant memzero_explicit()
Remove redundant memzero_explicit() in sun8i_ss_cipher() before calling
kfree_sensitive(). kfree_sensitive() will zero the memory with
memzero_explicit().
Signed-off-by: Denis Efremov
---
drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
index deb8b39a86db..ed2a69f82e1c 100644
--- a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
@@ -248,7 +248,6 @@ static int sun8i_ss_cipher(struct skcipher_request *areq)
offset = areq->cryptlen - ivsize;
if (rctx->op_dir & SS_DECRYPTION) {
memcpy(areq->iv, backup_iv, ivsize);
- memzero_explicit(backup_iv, ivsize);
kfree_sensitive(backup_iv);
} else {
scatterwalk_map_and_copy(areq->iv, areq->dst,
offset,
--
2.26.2
[PATCH] security/keys: use kvfree_sensitive()
Use kvfree_sensitive() instead of open-coding it.
Signed-off-by: Denis Efremov
---
security/keys/big_key.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 691347dea3c1..d17e5f09eeb8 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -121,8 +121,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
*path = file->f_path;
path_get(path);
fput(file);
- memzero_explicit(buf, enclen);
- kvfree(buf);
+ kvfree_sensitive(buf, enclen);
} else {
/* Just store the data in a buffer */
void *data = kmalloc(datalen, GFP_KERNEL);
@@ -140,8 +139,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
err_enckey:
kfree_sensitive(enckey);
error:
- memzero_explicit(buf, enclen);
- kvfree(buf);
+ kvfree_sensitive(buf, enclen);
return ret;
}
@@ -273,8 +271,7 @@ long big_key_read(const struct key *key, char *buffer,
size_t buflen)
err_fput:
fput(file);
error:
- memzero_explicit(buf, enclen);
- kvfree(buf);
+ kvfree_sensitive(buf, enclen);
} else {
ret = datalen;
memcpy(buffer, key->payload.data[big_key_data], datalen);
--
2.26.2
[PATCH v2 4/4] crypto: sun8i-ss - use kfree_sensitive()
Use kfree_sensitive() instead of open-coding it.
Signed-off-by: Denis Efremov
---
.../crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c | 15 +++
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
index 7b39b4495571..deb8b39a86db 100644
--- a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
@@ -368,10 +368,7 @@ void sun8i_ss_cipher_exit(struct crypto_tfm *tfm)
{
struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
crypto_free_skcipher(op->fallback_tfm);
pm_runtime_put_sync(op->ss->dev);
}
@@ -393,10 +390,7 @@ int sun8i_ss_aes_setkey(struct crypto_skcipher *tfm, const
u8 *key,
dev_dbg(ss->dev, "ERROR: Invalid keylen %u\n", keylen);
return -EINVAL;
}
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
op->keylen = keylen;
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
@@ -419,10 +413,7 @@ int sun8i_ss_des3_setkey(struct crypto_skcipher *tfm,
const u8 *key,
return -EINVAL;
}
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
op->keylen = keylen;
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
--
2.26.2
[PATCH v2 0/4] crypto: use kfree_sensitive()
kfree_sensitive() is introduced in commit 453431a54934
("mm, treewide: rename kzfree() to kfree_sensitive()") and uses
memzero_explicit() internally. Thus, we can switch to this API
instead of open-coding memzero_explicit() && kfree().
Changes in v2:
- if (op->len) check removed
Denis Efremov (4):
crypto: inside-secure - use kfree_sensitive()
crypto: amlogic - use kfree_sensitive()
crypto: sun8i-ce - use kfree_sensitive()
crypto: sun8i-ss - use kfree_sensitive()
.../crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c | 15 +++
.../crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c | 15 +++
drivers/crypto/amlogic/amlogic-gxl-cipher.c | 10 ++
drivers/crypto/inside-secure/safexcel_hash.c | 3 +--
4 files changed, 9 insertions(+), 34 deletions(-)
--
2.26.2
[PATCH v2 3/4] crypto: sun8i-ce - use kfree_sensitive()
Use kfree_sensitive() instead of open-coding it.
Signed-off-by: Denis Efremov
---
.../crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c | 15 +++
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index b4d5fea27d20..f996dc3d7dcc 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -366,10 +366,7 @@ void sun8i_ce_cipher_exit(struct crypto_tfm *tfm)
{
struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
crypto_free_skcipher(op->fallback_tfm);
pm_runtime_put_sync_suspend(op->ce->dev);
}
@@ -391,10 +388,7 @@ int sun8i_ce_aes_setkey(struct crypto_skcipher *tfm, const
u8 *key,
dev_dbg(ce->dev, "ERROR: Invalid keylen %u\n", keylen);
return -EINVAL;
}
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
op->keylen = keylen;
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
@@ -416,10 +410,7 @@ int sun8i_ce_des3_setkey(struct crypto_skcipher *tfm,
const u8 *key,
if (err)
return err;
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
op->keylen = keylen;
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
--
2.26.2
[PATCH v2 2/4] crypto: amlogic - use kfree_sensitive()
Use kfree_sensitive() instead of open-coding it.
Signed-off-by: Denis Efremov
---
drivers/crypto/amlogic/amlogic-gxl-cipher.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/crypto/amlogic/amlogic-gxl-cipher.c
b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
index d93210726697..ee5998af2fe8 100644
--- a/drivers/crypto/amlogic/amlogic-gxl-cipher.c
+++ b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
@@ -340,10 +340,7 @@ void meson_cipher_exit(struct crypto_tfm *tfm)
{
struct meson_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
crypto_free_skcipher(op->fallback_tfm);
}
@@ -367,10 +364,7 @@ int meson_aes_setkey(struct crypto_skcipher *tfm, const u8
*key,
dev_dbg(mc->dev, "ERROR: Invalid keylen %u\n", keylen);
return -EINVAL;
}
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
op->keylen = keylen;
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
--
2.26.2
