[PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive()

[Denis Efremov]

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);
 
if (ret)
return ret;
-- 
2.26.2

[PATCH] crypto: amlogic - use kfree_sensitive()

[Denis Efremov]

Use kfree_sensitive() instead of open-coding it.

Signed-off-by: Denis Efremov 
---
 drivers/crypto/amlogic/amlogic-gxl-cipher.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/amlogic/amlogic-gxl-cipher.c 
b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
index d93210726697..f3dca456d9f8 100644
--- a/drivers/crypto/amlogic/amlogic-gxl-cipher.c
+++ b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
@@ -341,8 +341,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);
 }
@@ -368,8 +367,7 @@ int meson_aes_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);
-- 
2.26.2

[PATCH] crypto: inside-secure - use kfree_sensitive()

[Denis Efremov]

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);
 
if (ret)
return ret;
-- 
2.26.2

Re: [PATCH] coccinelle: api: update kzfree script to kfree_sensitive

[Denis Efremov]

Ping?

On 8/11/20 10:49 AM, Denis Efremov wrote:
> Commit 453431a54934 ("mm, treewide: rename kzfree() to kfree_sensitive()")
> renames kzfree to kfree_sensitive and uses memzero_explicit(...) instead of
> memset(..., 0, ...) internally. Update cocci script to reflect these
> changes.
> 
> Signed-off-by: Denis Efremov 
> ---
> Julia, I think you can squash this commit with original script, or I can
> resend the whole script since it's not merged to the mainline.
> 
>  .../{kzfree.cocci => kfree_sensitive.cocci}   | 29 +--
>  1 file changed, 13 insertions(+), 16 deletions(-)
>  rename scripts/coccinelle/api/{kzfree.cocci => kfree_sensitive.cocci} (70%)
> 
> diff --git a/scripts/coccinelle/api/kzfree.cocci 
> b/scripts/coccinelle/api/kfree_sensitive.cocci
> similarity index 70%
> rename from scripts/coccinelle/api/kzfree.cocci
> rename to scripts/coccinelle/api/kfree_sensitive.cocci
> index 33625bd7cec9..e4a066a0b77d 100644
> --- a/scripts/coccinelle/api/kzfree.cocci
> +++ b/scripts/coccinelle/api/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 kfree_sensitive, kvfree_sensitive rather than memset or
> +/// memzero_explicit followed by kfree.
>  ///
>  // 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,8 @@ 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})
> @@ -56,17 +57,13 @@ 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);
> -//|
> +(
> +- kfree(E)@p;
> ++ kfree_sensitive(E);
> +|
>  - \(vfree\|kvfree\)(E)@p;
>  + kvfree_sensitive(E, size);
> -//)
> +)
>  
>  @rp_memset depends on patch@
>  expression E, size;
> @@ -80,7 +77,7 @@ type T;
>when strict
>  (
>  - kfree(E)@p;
> -+ kzfree(E);
> ++ kfree_sensitive(E);
>  |
>  - \(vfree\|kvfree\)(E)@p;
>  + kvfree_sensitive(E, size);
> @@ -91,11 +88,11 @@ p << r.p;
>  @@
>  
>  coccilib.report.print_report(p[0],
> -  "WARNING: opportunity for kzfree/kvfree_sensitive")
> +  "WARNING: opportunity for kfree_sensitive/kvfree_sensitive")
>  
>  @script:python depends on org@
>  p << r.p;
>  @@
>  
>  coccilib.org.print_todo(p[0],
> -  "WARNING: opportunity for kzfree/kvfree_sensitive")
> +  "WARNING: opportunity for kfree_sensitive/kvfree_sensitive")
> 

Re: [PATCH] seccomp: Use current_pt_regs()

[Denis Efremov]

On 8/24/20 7:34 PM, Kees Cook wrote:
> On Mon, Aug 24, 2020 at 03:59:21PM +0300, Denis Efremov wrote:
>> Modify seccomp_do_user_notification(), __seccomp_filter(),
>> __secure_computing() to use current_pt_regs().
> 
> This looks okay. It seems some architectures have a separate
> define for current_pt_regs(), though it's overlapped directly with
> task_pt_regs(). I'm curious what the benefit of the change is?
> 

Generally, it's just a shorthand.
From: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a3460a59747cfddfa7be4758e5ef08bf5d751d59
- arch versions are "optimized versions"
- some architectures have task_pt_regs() working only
  for traced tasks blocked on signal delivery. current_pt_regs()
  needs to work for *all* processes

My motivation:
I'm going to add cocci rule for using current_uid(), current_xxx(), ...
instead of raw accesses to current->cred, current->cred->uid
https://elixir.bootlin.com/linux/latest/source/include/linux/cred.h#L379
These interfaces use rcu_dereference_protected() internally for access
check.

I though that adding current_pt_regs(), current_user_stack_pointer() to
this cocci rule will be a good idea.

Thanks,
Denis

[PATCH] seccomp: Use current_pt_regs()

[Denis Efremov]

Modify seccomp_do_user_notification(), __seccomp_filter(),
__secure_computing() to use current_pt_regs().

Signed-off-by: Denis Efremov 
---
 kernel/seccomp.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 3ee59ce0a323..dc4eaa1d6002 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -910,7 +910,7 @@ static int seccomp_do_user_notification(int this_syscall,
if (flags & SECCOMP_USER_NOTIF_FLAG_CONTINUE)
return 0;
 
-   syscall_set_return_value(current, task_pt_regs(current),
+   syscall_set_return_value(current, current_pt_regs(),
 err, ret);
return -1;
 }
@@ -943,13 +943,13 @@ static int __seccomp_filter(int this_syscall, const 
struct seccomp_data *sd,
/* Set low-order bits as an errno, capped at MAX_ERRNO. */
if (data > MAX_ERRNO)
data = MAX_ERRNO;
-   syscall_set_return_value(current, task_pt_regs(current),
+   syscall_set_return_value(current, current_pt_regs(),
 -data, 0);
goto skip;
 
case SECCOMP_RET_TRAP:
/* Show the handler the original registers. */
-   syscall_rollback(current, task_pt_regs(current));
+   syscall_rollback(current, current_pt_regs());
/* Let the filter pass back 16 bits of data. */
seccomp_send_sigsys(this_syscall, data);
goto skip;
@@ -962,7 +962,7 @@ static int __seccomp_filter(int this_syscall, const struct 
seccomp_data *sd,
/* ENOSYS these calls if there is no tracer attached. */
if (!ptrace_event_enabled(current, PTRACE_EVENT_SECCOMP)) {
syscall_set_return_value(current,
-task_pt_regs(current),
+current_pt_regs(),
 -ENOSYS, 0);
goto skip;
}
@@ -982,7 +982,7 @@ static int __seccomp_filter(int this_syscall, const struct 
seccomp_data *sd,
if (fatal_signal_pending(current))
goto skip;
/* Check if the tracer forced the syscall to be skipped. */
-   this_syscall = syscall_get_nr(current, task_pt_regs(current));
+   this_syscall = syscall_get_nr(current, current_pt_regs());
if (this_syscall < 0)
goto skip;
 
@@ -1025,7 +1025,7 @@ static int __seccomp_filter(int this_syscall, const 
struct seccomp_data *sd,
kernel_siginfo_t info;
 
/* Show the original registers in the dump. */
-   syscall_rollback(current, task_pt_regs(current));
+   syscall_rollback(current, current_pt_regs());
/* Trigger a manual coredump since do_exit skips it. */
seccomp_init_siginfo(, this_syscall, data);
do_coredump();
@@ -1060,7 +1060,7 @@ int __secure_computing(const struct seccomp_data *sd)
return 0;
 
this_syscall = sd ? sd->nr :
-   syscall_get_nr(current, task_pt_regs(current));
+   syscall_get_nr(current, current_pt_regs());
 
switch (mode) {
case SECCOMP_MODE_STRICT:
-- 
2.26.2

[PATCH] namei: use current_fsuid() in may_follow_link()

[Denis Efremov]

Modify may_follow_link() to use current_fsuid()

Signed-off-by: Denis Efremov 
---
 fs/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index e99e2a9da0f7..1a47c9d8ce13 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -958,7 +958,7 @@ static inline int may_follow_link(struct nameidata *nd, 
const struct inode *inod
return 0;
 
/* Allowed if owner and follower match. */
-   if (uid_eq(current_cred()->fsuid, inode->i_uid))
+   if (uid_eq(current_fsuid(), inode->i_uid))
return 0;
 
/* Allowed if parent directory not sticky and world-writable. */
-- 
2.26.2

[PATCH] integrity: Use current_uid() in integrity_audit_message()

[Denis Efremov]

Modify integrity_audit_message() to use current_uid().

Signed-off-by: Denis Efremov 
---
 security/integrity/integrity_audit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/integrity_audit.c 
b/security/integrity/integrity_audit.c
index f25e7df099c8..29220056207f 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -47,7 +47,7 @@ void integrity_audit_message(int audit_msgno, struct inode 
*inode,
ab = audit_log_start(audit_context(), GFP_KERNEL, audit_msgno);
audit_log_format(ab, "pid=%d uid=%u auid=%u ses=%u",
 task_pid_nr(current),
-from_kuid(_user_ns, current_cred()->uid),
+from_kuid(_user_ns, current_uid()),
 from_kuid(_user_ns, audit_get_loginuid(current)),
 audit_get_sessionid(current));
audit_log_task_context(ab);
-- 
2.26.2

[PATCH] virt: vbox: Use current_uid() in vbg_misc_device_requestor()

[Denis Efremov]

Modify vbg_misc_device_requestor() to use current_uid() wrapper.

Signed-off-by: Denis Efremov 
---
 drivers/virt/vboxguest/vboxguest_linux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virt/vboxguest/vboxguest_linux.c 
b/drivers/virt/vboxguest/vboxguest_linux.c
index 32c2c52f7e84..6215a688edaf 100644
--- a/drivers/virt/vboxguest/vboxguest_linux.c
+++ b/drivers/virt/vboxguest/vboxguest_linux.c
@@ -35,7 +35,7 @@ static u32 vbg_misc_device_requestor(struct inode *inode)
VMMDEV_REQUESTOR_CON_DONT_KNOW |
VMMDEV_REQUESTOR_TRUST_NOT_GIVEN;
 
-   if (from_kuid(current_user_ns(), current->cred->uid) == 0)
+   if (from_kuid(current_user_ns(), current_uid()) == 0)
requestor |= VMMDEV_REQUESTOR_USR_ROOT;
else
requestor |= VMMDEV_REQUESTOR_USR_USER;
-- 
2.26.2

[PATCH] security/commoncap: Use current_user_ns()

[Denis Efremov]

Modify cap_inh_is_capped(), cap_task_prctl() to use current_user_ns().

Signed-off-by: Denis Efremov 
---
 security/commoncap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 59bf3c1674c8..82a61f77c07c 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -220,7 +220,7 @@ static inline int cap_inh_is_capped(void)
/* they are so limited unless the current task has the CAP_SETPCAP
 * capability
 */
-   if (cap_capable(current_cred(), current_cred()->user_ns,
+   if (cap_capable(current_cred(), current_user_ns(),
CAP_SETPCAP, CAP_OPT_NONE) == 0)
return 0;
return 1;
@@ -1206,7 +1206,7 @@ int cap_task_prctl(int option, unsigned long arg2, 
unsigned long arg3,
|| ((old->securebits & SECURE_ALL_LOCKS & ~arg2))   /*[2]*/
|| (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS))   /*[3]*/
|| (cap_capable(current_cred(),
-   current_cred()->user_ns,
+   current_user_ns(),
CAP_SETPCAP,
CAP_OPT_NONE) != 0) /*[4]*/
/*
-- 
2.26.2

[PATCH v2] coccinelle: api: add kobj_to_dev.cocci script

[Denis Efremov]

Use kobj_to_dev() instead of container_of().

Signed-off-by: Denis Efremov 
---
Changes in v2:
 - "symbol kobj;" added to the rule r

 scripts/coccinelle/api/kobj_to_dev.cocci | 45 
 1 file changed, 45 insertions(+)
 create mode 100644 scripts/coccinelle/api/kobj_to_dev.cocci

diff --git a/scripts/coccinelle/api/kobj_to_dev.cocci 
b/scripts/coccinelle/api/kobj_to_dev.cocci
new file mode 100644
index ..cd5d31c6fe76
--- /dev/null
+++ b/scripts/coccinelle/api/kobj_to_dev.cocci
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Use kobj_to_dev() instead of container_of()
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: kobj_to_dev, container_of
+//
+
+virtual context
+virtual report
+virtual org
+virtual patch
+
+
+@r depends on !patch@
+expression ptr;
+symbol kobj;
+position p;
+@@
+
+* container_of(ptr, struct device, kobj)@p
+
+
+@depends on patch@
+expression ptr;
+@@
+
+- container_of(ptr, struct device, kobj)
++ kobj_to_dev(ptr)
+
+
+@script:python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for kobj_to_dev()")
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for kobj_to_dev()")
-- 
2.26.2

[PATCH] coccinelle: api: add kobj_to_dev.cocci script

[Denis Efremov]

Use kobj_to_dev() instead of container_of().

Signed-off-by: Denis Efremov 
---
Examples of such patches:
893c3d82b425 watchdog: Use kobj_to_dev() API
23fd63a44460 hwmon: (nct6683) Replace container_of() with  kobj_to_dev()
224941c9424f power: supply: use kobj_to_dev
a9b9b2af40c7 backlight: lm3533_bl: Use kobj_to_dev() instead
0acb47a3a093 qlcnic: Use kobj_to_dev() instead
97cd738c44c8 gpiolib: sysfs: use kobj_to_dev
d06f9e6c8960 hwmon: (nct7802) Replace container_of() API
036855a4c3b3 hwmon : (nct6775) Use kobj_to_dev() API
baf1d9c18293 driver/base/soc: Use kobj_to_dev() API
ae243ef0afbc rtc: sysfs: use kobj_to_dev
6b060d8a09e9 i2c: use kobj_to_dev() API
9e7bd945b9a9 scsi: core: use kobj_to_dev
0d730b57b95f s390/cio: use kobj_to_dev() API
0616ca73fd35 usb: use kobj_to_dev() API
8c9b839c0b80 alpha: use kobj_to_dev()
016c0bbae1d1 netxen: Use kobj_to_dev()
6908b45eafc4 GenWQE: use kobj_to_dev()
85f4f39c80e9 pch_phub: use kobj_to_dev()
47679cde604d misc: c2port: use kobj_to_dev()
85016ff33f35 misc: cxl: use kobj_to_dev()
092462c2b522 misc: eeprom: use kobj_to_dev()
a9c9d9aca4e7 zorro: Use kobj_to_dev()
a253f1eee6c4 rapidio: use kobj_to_dev()
e3837b00b6bb drm/radeon: use kobj_to_dev()
cc29ec874b37 drm/amdgpu: use kobj_to_dev()
d122cbf1a310 drm/sysfs: use kobj_to_dev()
657fb5fbadb3 drm/i915: use kobj_to_dev()
554a60379aaa PCI: Use kobj_to_dev() instead of open-coding it
2cf83833fc9c HID: use kobj_to_dev()
aeb7ed14fe5d bridge: use kobj_to_dev instead of to_dev
8e3829c61b48 staging:iio:adis16220: Use kobj_to_dev instead of open-coding it
b0d1f807f340 driver-core: Use kobj_to_dev instead of re-implementing it

 scripts/coccinelle/api/kobj_to_dev.cocci | 44 
 1 file changed, 44 insertions(+)
 create mode 100644 scripts/coccinelle/api/kobj_to_dev.cocci

diff --git a/scripts/coccinelle/api/kobj_to_dev.cocci 
b/scripts/coccinelle/api/kobj_to_dev.cocci
new file mode 100644
index ..e2cdd424aeca
--- /dev/null
+++ b/scripts/coccinelle/api/kobj_to_dev.cocci
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Use kobj_to_dev() instead of container_of()
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: kobj_to_dev, container_of
+//
+
+virtual context
+virtual report
+virtual org
+virtual patch
+
+
+@r depends on !patch@
+expression ptr;
+position p;
+@@
+
+* container_of(ptr, struct device, kobj)@p
+
+
+@depends on patch@
+expression ptr;
+@@
+
+- container_of(ptr, struct device, kobj)
++ kobj_to_dev(ptr)
+
+
+@script:python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for kobj_to_dev()")
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for kobj_to_dev()")
-- 
2.26.2

Re: [PATCH] coccinelle: api: add sprintf() support to device_attr_show

[Denis Efremov]

On 8/14/20 3:30 PM, Markus Elfring wrote:
>>> You propose to use a nested SmPL disjunction for desired adjustments.
>>> I suggest to start a corresponding case distinction behind
>>> the key word “return” instead of repeating it three times.
>>
>> It doesn't work.
> 
> How do you think about to apply a SmPL rule variant like the following?
> 
> @rp depends on patch@
> identifier show, dev, attr, buf;
> constant str;
> @@
> 
> ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
> {
>   <...
>   return
> (
> - snprintf
> + sprintf
>   (
>   buf,
> - \(PAGE_SIZE\|PAGE_SIZE - 1\),
> ( str
> |
> ( "%i"\|"%i\n"\|"%li"\|"%li\n"\|"%lli"\|"%lli\n"\|
>   "%d"\|"%d\n"\|"%ld"\|"%ld\n"\|"%lld"\|"%lld\n"\|
>   "%u"\|"%u\n"\|"%lu"\|"%lu\n"\|"%llu"\|"%llu\n"\|
>   "%x"\|"%x\n"\|"%lx"\|"%lx\n"\|"%llx"\|"%llx\n"\|
>   "%X"\|"%X\n"\|"%lX"\|"%lX\n"\|"%llX"\|"%llX\n"\|
>   
> "0x%x"\|"0x%x\n"\|"0x%lx"\|"0x%lx\n"\|"0x%llx"\|"0x%llx\n"\|
>   
> "0x%X"\|"0x%X\n"\|"0x%lX"\|"0x%lX\n"\|"0x%llX"\|"0x%llX\n"\|
>   "%02x\n"\|"%03x\n"\|"%04x\n"\|"%08x\n"\|
>   "%02X\n"\|"%03X\n"\|"%04X\n"\|"%08X\n"\|
>   "0x%02x\n"\|"0x%03x\n"\|"0x%04x\n"\|"0x%08x\n"\|
>   "0x%02X\n"\|"0x%03X\n"\|"0x%04X\n"\|"0x%08X\n"\|
>   "%zd"\|"%zd\n"\|"%zu"\|"%zu\n"\|"%zx"\|"%zx\n"\|
>   "%c"\|"%c\n"\|"%p"\|"%p\n"\|"%pU\n"\|"%pUl\n"\|"%hu\n"
> ) ,
>   ...
> )
>   )
> |
> - snprintf
> + scnprintf
>   (...)
> );
>   ...>
> }
> 

3 levels of nested disjunctions makes this pattern completely unreadable
and gives no comparable benefits. I don't think we should care much about
number of characters in the kernel sources, gzip will do a better job
anyway.


Thanks,
Denis

Re: [PATCH] coccinelle: api: add sprintf() support to device_attr_show

[Denis Efremov]

Hi,

Markus, I think that CCing new people and spam them with mails they
are obviously not interested in doesn't bring an additional value to
the discussion. linux-kernel and cocci mailing lists are enough
in my opinion. This also will allow us to keep "threaded" mail
order.

On 8/14/20 11:30 AM, Markus Elfring wrote:
>> Interesting enough that with this patch coccinelle starts to skip
>> patch generation in some cases. For example, it skips patch for
>> drivers/base/core.c This is an unexpected result for me.
> 
> Would you like to point questionable differences for such patch hunks out?

Without this patch the script generates:
$ spatch -D patch --no-includes --include-headers --cocci-file 
scripts/coccinelle/api/device_attr_show.cocci drivers/base/core.c
--- drivers/base/core.c
+++ /tmp/cocci-output-63510-2f17ff-core.c
@@ -1713,7 +1713,7 @@ ssize_t device_show_ulong(struct device
char *buf)
{
struct dev_ext_attribute *ea = to_ext_attr(attr);
-   return snprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
+   return scnprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
}
EXPORT_SYMBOL_GPL(device_show_ulong);

@@ -1743,7 +1743,7 @@ ssize_t device_show_int(struct device *d
{
struct dev_ext_attribute *ea = to_ext_attr(attr);

-   return snprintf(buf, PAGE_SIZE, "%d\n", *(int *)(ea->var));
+   return scnprintf(buf, PAGE_SIZE, "%d\n", *(int *)(ea->var));
}
EXPORT_SYMBOL_GPL(device_show_int);

@@ -1764,7 +1764,7 @@ ssize_t device_show_bool(struct device *
{
struct dev_ext_attribute *ea = to_ext_attr(attr);

-   return snprintf(buf, PAGE_SIZE, "%d\n", *(bool *)(ea->var));
+   return scnprintf(buf, PAGE_SIZE, "%d\n", *(bool *)(ea->var));
}
EXPORT_SYMBOL_GPL(device_show_bool);

With this patch it generates nothing. I would expect spatch to generate
a different patch with sprintf instead of scnprintf, because I think 
... is enough to match "*(int *)(ea->var)". Even if it can't match sprintf
pattern it should fallback to scnprintf pattern.

> You propose to use a nested SmPL disjunction for desired adjustments.
> I suggest to start a corresponding case distinction behind
> the key word “return” instead of repeating it three times.

It doesn't work.

Thanks,
Denis

[PATCH] coccinelle: api: add sprintf() support to device_attr_show

[Denis Efremov]

It's safe to use sprintf() for simple cases in device_attr_show
type of functions. Add support for sprintf() in patch mode to
the device_attr_show.cocci script to print numbers and pointers.

Signed-off-by: Denis Efremov 
---
Interesting enough that with this patch coccinelle starts to skip
patch generation in some cases. For example, it skips patch for
drivers/base/core.c This is an unexpected result for me.

 scripts/coccinelle/api/device_attr_show.cocci | 30 +++
 1 file changed, 30 insertions(+)

diff --git a/scripts/coccinelle/api/device_attr_show.cocci 
b/scripts/coccinelle/api/device_attr_show.cocci
index d8ec4bb8ac41..1248b8c76cfe 100644
--- a/scripts/coccinelle/api/device_attr_show.cocci
+++ b/scripts/coccinelle/api/device_attr_show.cocci
@@ -30,15 +30,45 @@ ssize_t show(struct device *dev, struct device_attribute 
*attr, char *buf)
 
 @rp depends on patch@
 identifier show, dev, attr, buf;
+constant str;
 @@
 
 ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
 {
<...
+(
+   return
+-  snprintf
++  sprintf
+   (buf,
+-  \(PAGE_SIZE\|PAGE_SIZE - 1\),
+   str);
+|
+   return
+-  snprintf
++  sprintf
+   (buf,
+-  \(PAGE_SIZE\|PAGE_SIZE - 1\),
+   \("%i"\|"%i\n"\|"%li"\|"%li\n"\|"%lli"\|"%lli\n"\|
+ "%d"\|"%d\n"\|"%ld"\|"%ld\n"\|"%lld"\|"%lld\n"\|
+ "%u"\|"%u\n"\|"%lu"\|"%lu\n"\|"%llu"\|"%llu\n"\|
+ "%x"\|"%x\n"\|"%lx"\|"%lx\n"\|"%llx"\|"%llx\n"\|
+ "%X"\|"%X\n"\|"%lX"\|"%lX\n"\|"%llX"\|"%llX\n"\|
+ 
"0x%x"\|"0x%x\n"\|"0x%lx"\|"0x%lx\n"\|"0x%llx"\|"0x%llx\n"\|
+ 
"0x%X"\|"0x%X\n"\|"0x%lX"\|"0x%lX\n"\|"0x%llX"\|"0x%llX\n"\|
+ "%02x\n"\|"%03x\n"\|"%04x\n"\|"%08x\n"\|
+ "%02X\n"\|"%03X\n"\|"%04X\n"\|"%08X\n"\|
+ "0x%02x\n"\|"0x%03x\n"\|"0x%04x\n"\|"0x%08x\n"\|
+ "0x%02X\n"\|"0x%03X\n"\|"0x%04X\n"\|"0x%08X\n"\|
+ "%zd"\|"%zd\n"\|"%zu"\|"%zu\n"\|"%zx"\|"%zx\n"\|
+ 
"%c"\|"%c\n"\|"%p"\|"%p\n"\|"%pU\n"\|"%pUl\n"\|"%hu\n"\),
+   ...);
+|
return
 -  snprintf
 +  scnprintf
(...);
+)
...>
 }
 
-- 
2.26.2

[RFC PATCH] coccinelle: misc: add uninitialized_var.cocci script

[Denis Efremov]

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 
---
List of warnings:
./lib/glob.c:48:31-39: WARNING: this kind of initialization is deprecated
./tools/testing/selftests/vm/userfaultfd.c:349:15-22: WARNING: this kind of 
initialization is deprecated
./drivers/block/drbd/drbd_vli.h:330:5-9: WARNING: this kind of initialization 
is deprecated
./drivers/char/hw_random/intel-rng.c:333:15-18: WARNING: this kind of 
initialization is deprecated
./drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c:316:7-10: WARNING: this 
kind of initialization is deprecated
./arch/x86/include/asm/paravirt_types.h:455:15-20: WARNING: this kind of 
initialization is deprecated
./arch/x86/include/asm/paravirt_types.h:455:30-35: WARNING: this kind of 
initialization is deprecated
./arch/x86/include/asm/paravirt_types.h:455:45-50: WARNING: this kind of 
initialization is deprecated
./arch/x86/include/asm/paravirt_types.h:475:15-20: WARNING: this kind of 
initialization is deprecated
./arch/x86/include/asm/paravirt_types.h:475:30-35: WARNING: this kind of 
initialization is deprecated
./arch/x86/include/asm/paravirt_types.h:476:2-7: WARNING: this kind of 
initialization isdeprecated
./arch/x86/include/asm/paravirt_types.h:476:17-22: WARNING: this kind of 
initialization is deprecated
./arch/x86/include/asm/paravirt_types.h:476:32-37: WARNING: this kind of 
initialization is deprecated

 .../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 ..e4787bc6ab9c
--- /dev/null
+++ b/scripts/coccinelle/misc/uninitialized_var.cocci
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// uninitialized_var() and its open-coded variations are
+/// deprecated. For details, see:
+/// Documentation/process/deprecated.rst
+///
+// 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));
+//|
+// TODO: Actually, I'm not sure about this pattern.
+// Looks like it's used in wireless drivers to determine
+// whether data belongs to the driver or not.
+// Here are all matches:
+// https://elixir.bootlin.com/linux/latest/source/net/mac802154/util.c#L14
+// 
https://elixir.bootlin.com/linux/latest/source/drivers/staging/wlan-ng/cfg80211.c#L48
+// 
https://elixir.bootlin.com/linux/latest/source/drivers/net/wireless/intersil/orinoco/cfg.c#L21
+// https://elixir.bootlin.com/linux/latest/source/net/mac80211/util.c#L37
+// 
https://elixir.bootlin.com/linux/latest/source/drivers/net/wireless/rndis_wlan.c#L544
+// * T *var@p = 
+)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+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")
-- 
2.26.2

[PATCH] coccinelle: api: update kzfree script to kfree_sensitive

[Denis Efremov]

Commit 453431a54934 ("mm, treewide: rename kzfree() to kfree_sensitive()")
renames kzfree to kfree_sensitive and uses memzero_explicit(...) instead of
memset(..., 0, ...) internally. Update cocci script to reflect these
changes.

Signed-off-by: Denis Efremov 
---
Julia, I think you can squash this commit with original script, or I can
resend the whole script since it's not merged to the mainline.

 .../{kzfree.cocci => kfree_sensitive.cocci}   | 29 +--
 1 file changed, 13 insertions(+), 16 deletions(-)
 rename scripts/coccinelle/api/{kzfree.cocci => kfree_sensitive.cocci} (70%)

diff --git a/scripts/coccinelle/api/kzfree.cocci 
b/scripts/coccinelle/api/kfree_sensitive.cocci
similarity index 70%
rename from scripts/coccinelle/api/kzfree.cocci
rename to scripts/coccinelle/api/kfree_sensitive.cocci
index 33625bd7cec9..e4a066a0b77d 100644
--- a/scripts/coccinelle/api/kzfree.cocci
+++ b/scripts/coccinelle/api/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 kfree_sensitive, kvfree_sensitive rather than memset or
+/// memzero_explicit followed by kfree.
 ///
 // 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,8 @@ 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})
@@ -56,17 +57,13 @@ 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);
-//|
+(
+- kfree(E)@p;
++ kfree_sensitive(E);
+|
 - \(vfree\|kvfree\)(E)@p;
 + kvfree_sensitive(E, size);
-//)
+)
 
 @rp_memset depends on patch@
 expression E, size;
@@ -80,7 +77,7 @@ type T;
   when strict
 (
 - kfree(E)@p;
-+ kzfree(E);
++ kfree_sensitive(E);
 |
 - \(vfree\|kvfree\)(E)@p;
 + kvfree_sensitive(E, size);
@@ -91,11 +88,11 @@ p << r.p;
 @@
 
 coccilib.report.print_report(p[0],
-  "WARNING: opportunity for kzfree/kvfree_sensitive")
+  "WARNING: opportunity for kfree_sensitive/kvfree_sensitive")
 
 @script:python depends on org@
 p << r.p;
 @@
 
 coccilib.org.print_todo(p[0],
-  "WARNING: opportunity for kzfree/kvfree_sensitive")
+  "WARNING: opportunity for kfree_sensitive/kvfree_sensitive")
-- 
2.26.2

Re: [PATCH v4] coccinelle: api: add kzfree script

[Denis Efremov]

On 8/11/20 2:45 AM, Eric Biggers wrote:
> On Fri, Jul 17, 2020 at 10:39:20PM +0200, Julia Lawall wrote:
>>
>>
>> On Fri, 17 Jul 2020, Denis Efremov wrote:
>>
>>> Check for memset()/memzero_explicit() followed by kfree()/vfree()/kvfree().
>>>
>>> Signed-off-by: Denis Efremov 
>>
>> Applied.
> 
> FYI, this new script is already outdated, since kzfree() has been renamed to
> kfree_sensitive().
> 

Ok, I will send an update.

Thanks,Denis

[PATCH v2] coccinelle: misc: add flexible_array.cocci script

[Denis Efremov]

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@
+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)
-- 
2.26.2

[RFC PATCH] coccinelle: misc: add flexible_array.cocci script

[Denis Efremov]

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 
---

Currently, it's just a draft. I've placed a number of questions in the
script and marked them as TODO. Kees, Gustavo, if you could help me with
my questions I think that this rule will be enough to close:
https://github.com/KSPP/linux/issues/76

BTW, I it's possible to not warn about files in uapi folder if
this is relevant. Do I need to do it in the script?

 scripts/coccinelle/misc/flexible_array.cocci | 158 +++
 1 file changed, 158 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 ..1e7165c79e60
--- /dev/null
+++ b/scripts/coccinelle/misc/flexible_array.cocci
@@ -0,0 +1,158 @@
+// 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
+
+@r depends on !patch@
+identifier name, size, array;
+// TODO: We can additionally restrict size and array to:
+// identifier size =~ ".*(num|len|count|size|ncpus).*";
+// identifier array !~ ".*(pad|reserved).*";
+// Do we need it?
+type TS, TA;
+position p;
+@@
+
+(
+  // This will also match: typedef struct name { ...
+  // However nested structs are not matched, i.e.:
+  //   struct name1 { struct name2 { int s; int a[0]; } st; int i; }
+  // will not be matched. Do we need to handle it?
+  struct name {
+...  // TODO: Maybe simple ... is enough? It will match structs with a
+TS size; // single field, e.g.
+...  // 
https://elixir.bootlin.com/linux/v5.8/source/arch/arm/include/uapi/asm/setup.h#L127
+(
+*TA array@p[0];
+|
+ // TODO: It seems that there are exception cases for array[1], e.g.
+ //  
https://elixir.bootlin.com/linux/v5.8/source/arch/powerpc/boot/rs6000.h#L152
+ //  
https://elixir.bootlin.com/linux/v5.8/source/include/uapi/linux/cdrom.h#L292
+ //  
https://elixir.bootlin.com/linux/v5.8/source/drivers/net/wireless/ath/ath6kl/usb.c#L108
+ // We could either drop array[1] checking from this rule or
+ // restrict array name with regexp and add, for example, an "allowlist"
+ // with struct names where we allow this code pattern.
+ // TODO: How to handle: u8 data[1][MAXLEN_PSTR6]; ?
+*TA array@p[1];
+)
+  };
+|
+  struct {
+...
+TS size;
+...
+(
+*TA array@p[0];
+|
+*TA array@p[1];
+)
+  };
+|
+  // TODO: do we need to handle unions?
+  union name {
+...
+TS size;
+...
+(
+*TA array@p[0];
+|
+*TA array@p[1];
+)
+  };
+|
+  union {
+...
+TS size;
+...
+(
+*TA array@p[0];
+|
+*TA array@p[1];
+)
+  };
+)
+
+// FIXME: Patch mode doesn't work as expected.
+// Coccinelle handles formatting incorrectly.
+// Patch mode in this rule should be disabled until
+// proper formatting will be supported.
+@depends on patch exists@
+identifier name, size, array;
+type TS, TA;
+@@
+
+(
+  struct name {
+...
+TS size;
+...
+(
+-TA array[0];
+|
+-TA array[1];
+)
++TA array[];
+  };
+|
+  struct {
+...
+TS size;
+...
+(
+-TA array[0];
+|
+-TA array[1];
+)
++TA array[];
+  };
+|
+  union name {
+...
+TS size;
+...
+(
+-TA array[0];
+|
+-TA array[1];
+)
++TA array[];
+  };
+|
+  union {
+...
+TS size;
+...
+(
+-TA array[0];
+|
+-TA array[1];
+)
++TA array[];
+  };
+)
+
+@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)
-- 
2.26.2

[PATCH v3] coccinelle: api: add kvmalloc script

[Denis Efremov]

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

 scripts/coccinelle/api/kvmalloc.cocci | 188 ++
 1 file changed, 188 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 ..2cb9281cc092
--- /dev/null
+++ b/scripts/coccinelle/api/kvmalloc.cocci
@@ -0,0 +1,188 @@
+// 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;
+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, ...)
+...
+  } 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, ...)
+  ... 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, ...);
+  ... 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, flags, size, node;
+binary operator cmp = {<=, <, ==, >, >=};
+identifier x;
+type T;
+@@
+
+(
+- if (size cmp E1)
+-E = kmalloc(size, flags);
+- else
+-E = vmalloc(size);
++ E = kvmalloc(size, flags);
+|
+- E = kmalloc(size, flags | __GFP_NOWARN);
+- if (E == NULL)
+-   E = vmalloc(size);
++ E = kvmalloc(size, flags);
+|
+- T x = kmalloc(size, flags | __GFP_NOWARN);
+- if (x == NULL)
+-   x = vmalloc(size);
++ T x = kvmalloc(size, flags);
+|
+- if (size cmp E1)
+-E = kzalloc(size, flags);
+- else
+-E = vzalloc(size);
++ E = kvzalloc(size, flags);
+|
+- E = kzalloc(size, flags | __GFP_NOWARN);
+- if (E == NULL)
+-   E = vzalloc(size);
++ E = kvzalloc(size, flags);
+|
+- T x = kzalloc(size, flags | __GFP_NOWARN);
+- if (x == NULL)
+-   x = vzalloc(size);
++ T x = kvzalloc(size, flags);
+|
+- if (size cmp E1)
+-E = kmalloc_node(size, flags, node);
+- else
+-E = vmalloc_node(size, node);
++ E = kvmalloc_node(size, flags, node);
+|
+- E = kmalloc_node(size, flags | __GFP_NOWARN, node);
+- if (E == NULL)
+-   E = vmalloc_node(size, node);
++ E = kvmalloc_node(size, flags, node);
+|
+- T x = kmalloc_node(size, flags | __GFP_NOWARN, node);
+- if (x == NULL)
+-   x = vmalloc_node(size, node);
++ T x = kvmalloc_node(size, flags, node);
+|
+- if (size cmp E1)
+-E = kvzalloc_node(size, flags, node);
+- else
+-E = vzalloc_node(size, node);
++ E = kvzalloc_node(size, flags, node);
+|
+- E = kvzalloc_node(size, flags | __GFP_NOWARN, node);
+- if (E == NULL)
+-   E = vzalloc_node(size, node);
++ E = kvzalloc_node(size, flags, node);
+|
+- T x = kvzalloc_node(size, flags | __GFP_NOWARN, node);
+- if (x == NULL)
+-   x = vzalloc_node(size, node);
++ T x = kvzalloc_node(size, flags, node);
+)
+
+@depends on patch@
+expression E;
+position p : script:python() { relevant(p) };
+@@
+
+- if (is_vmalloc_addr(E))@p
+-   vfree(E);
+- else
+-   kfree(E);
++ kvfree(E);
+
+@script: python depends on report@
+p << kvmalloc.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING: opportunity for kvmalloc")
+
+@script: python depends on org@
+p << kvmalloc.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING: opportunity for kvmalloc")
+
+@script: python depends on report@
+p << kvfree.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING: opportunity for kvfree")
+
+@script: python depends on org@
+p << kvfree.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING: opportunity for kvfree")
-- 
2.26.2

[PATCH v2] coccinelle: api: add kvmalloc script

[Denis Efremov]

Suggest kvmalloc instead of opencoded kmalloc && vmalloc condition.

Signed-off-by: Denis Efremov 
---
Changes in v2:
 - binary operator cmp added
 - NULL comparisions simplified
 - "T x" case added to !patch mode

 scripts/coccinelle/api/kvmalloc.cocci | 142 ++
 1 file changed, 142 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 ..20b22e3d0f74
--- /dev/null
+++ b/scripts/coccinelle/api/kvmalloc.cocci
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Find if/else condition with kmalloc/vmalloc calls.
+/// Suggest to use kvmalloc instead.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual patch
+virtual report
+virtual org
+virtual context
+
+@opportunity depends on !patch@
+expression E, E1, size;
+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, ...)
+...
+  } 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, ...)
+  ... 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, ...);
+  ... when != x = E1
+  when != size = E1
+  when any
+* if (x == NULL)@p {
+...
+*   x = \(vmalloc\|vzalloc\|vmalloc_node\|vzalloc_node\)(..., size, ...)
+...
+  }
+)
+
+@depends on patch@
+expression E, E1, flags, size, node;
+binary operator cmp = {<=, <, ==, >, >=};
+identifier x;
+type T;
+@@
+
+(
+- if (size cmp E1)
+-E = kmalloc(size, flags);
+- else
+-E = vmalloc(size);
++ E = kvmalloc(size, flags);
+|
+- E = kmalloc(size, flags | __GFP_NOWARN);
+- if (E == NULL)
+-   E = vmalloc(size);
++ E = kvmalloc(size, flags);
+|
+- T x = kmalloc(size, flags | __GFP_NOWARN);
+- if (x == NULL)
+-   x = vmalloc(size);
++ T x = kvmalloc(size, flags);
+|
+- if (size cmp E1)
+-E = kzalloc(size, flags);
+- else
+-E = vzalloc(size);
++ E = kvzalloc(size, flags);
+|
+- E = kzalloc(size, flags | __GFP_NOWARN);
+- if (E == NULL)
+-   E = vzalloc(size);
++ E = kvzalloc(size, flags);
+|
+- T x = kzalloc(size, flags | __GFP_NOWARN);
+- if (x == NULL)
+-   x = vzalloc(size);
++ T x = kvzalloc(size, flags);
+|
+- if (size cmp E1)
+-E = kmalloc_node(size, flags, node);
+- else
+-E = vmalloc_node(size, node);
++ E = kvmalloc_node(size, flags, node);
+|
+- E = kmalloc_node(size, flags | __GFP_NOWARN, node);
+- if (E == NULL)
+-   E = vmalloc_node(size, node);
++ E = kvmalloc_node(size, flags, node);
+|
+- T x = kmalloc_node(size, flags | __GFP_NOWARN, node);
+- if (x == NULL)
+-   x = vmalloc_node(size, node);
++ T x = kvmalloc_node(size, flags, node);
+|
+- if (size cmp E1)
+-E = kvzalloc_node(size, flags, node);
+- else
+-E = vzalloc_node(size, node);
++ E = kvzalloc_node(size, flags, node);
+|
+- E = kvzalloc_node(size, flags | __GFP_NOWARN, node);
+- if (E == NULL)
+-   E = vzalloc_node(size, node);
++ E = kvzalloc_node(size, flags, node);
+|
+- T x = kvzalloc_node(size, flags | __GFP_NOWARN, node);
+- if (x == NULL)
+-   x = vzalloc_node(size, node);
++ T x = kvzalloc_node(size, flags, node);
+)
+
+@script: python depends on report@
+p << opportunity.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING: opportunity for kvmalloc")
+
+@script: python depends on org@
+p << opportunity.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING: opportunity for kvmalloc")
-- 
2.26.2

[PATCH v7] coccinelle: api: add kfree_mismatch script

[Denis Efremov]

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)

 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 ..9e9ef9fd7a25
--- /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\|kzfree\|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
++ vfree(E)
+
+@kvfree depends on !patch@
+expression E;
+position a, f;
+@@
+
+* E = \(kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|
+*   kvmalloc_array\)(...)@a
+  ... when !=

Re: [PATCH v6] coccinelle: api: add kvfree script

[Denis Efremov]

Is there a difference from cocci point of view between:

... when != !is_vmalloc_addr(E)

and 

... when != is_vmalloc_addr(E)

Should the latter one be used in most cases?

Thanks,
Denis

Re: [PATCH v6] coccinelle: api: add kvfree script

[Denis Efremov]

On 8/2/20 11:24 PM, Julia Lawall wrote:
>> +@initialize:python@
>> +@@
>> +# low-level memory api
>> +filter = frozenset(['__vmalloc_area_node'])
>> +
>> +def relevant(p):
>> +return not (filter & {el.current_element for el in p})
> 
> Is this used?

I'll remove it in v8. Or do you want me to add iterate_dir_item() in the list?

> 
> Otherwise, I think it would be good to not warn about a use of kvfree
> if that use is reachable from a kvmalloc.  There seems to be such a false
> positive in fs/btrfs/send.c, on line 1118.

I don't know how to handle this case without position filter.
It's too complex. In iterate_dir_item() there is:
buf = kmalloc(buf_len, GFP_KERNEL);
while(...) {
if (...) {
if (is_vmalloc_addr(buf)) {
vfree(buf);
...
} else {
char *tmp = krealloc(buf, ...);

if (!tmp)
kfree(buf);
...
}
if (!buf) {
buf = kvmalloc(buf_len, GFP_KERNEL);
...
}
}
}
kvfree(buf);

Adding "when != kvfree(E)" is not enough:
* E = \(kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|
*   kvmalloc_array\)(...)@k
... when != is_vmalloc_addr(E)
+   when != kvfree(E)
when any
* \(kfree\|kzfree\|vfree\|vfree_atomic\)(E)@p

> 
> It also seems that when there are both a kmalloc and a vmalloc, there is
> no warning if kfree or vfree is used.  Is that intentional?
> 

No, I will try to address it in v8.

Regards,
Denis

[PATCH] coccinelle: api: add kvmalloc script

[Denis Efremov]

Suggest kvmalloc instead of opencoded kmalloc && vmalloc condition.

Signed-off-by: Denis Efremov 
---

If coccinelle fails with "Segmentation fault" during analysis, then
one needs to increase stack limit, e.g. ulimit -s 32767.

Current, I've sent only one patch for this rule and will send the rest
after the merge window. https://lkml.org/lkml/2020/7/31/986

 scripts/coccinelle/api/kvmalloc.cocci | 127 ++
 1 file changed, 127 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 ..76d6aeab7c09
--- /dev/null
+++ b/scripts/coccinelle/api/kvmalloc.cocci
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Find conditions in code for kmalloc/vmalloc calls.
+/// Suggest to use kvmalloc instead.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual patch
+virtual report
+virtual org
+virtual context
+
+@opportunity depends on !patch@
+expression E, E1, size;
+position p;
+@@
+
+(
+* if (\(size <= E1\|size < E1\|size == E1\|size > E1\) || ...)@p {
+...
+*E = \(kmalloc\|kzalloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+*  kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(..., size, ...)
+...
+  } 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, ...)
+  ... when != E = E1
+  when != size = E1
+  when any
+* if (\(!E\|E == NULL\))@p {
+...
+*   E = \(vmalloc\|vzalloc\|vmalloc_node\|vzalloc_node\)(..., size, ...)
+...
+  }
+)
+
+@depends on patch@
+expression E, E1, flags, size, node;
+identifier x;
+type T;
+@@
+
+(
+- if (\(size <= E1\|size < E1\|size == E1\|size > E1\))
+-E = kmalloc(size, flags);
+- else
+-E = vmalloc(size);
++ E = kvmalloc(size, flags);
+|
+- E = kmalloc(size, flags | __GFP_NOWARN);
+- if (\(!E\|E == NULL\))
+-   E = vmalloc(size);
++ E = kvmalloc(size, flags);
+|
+- T x = kmalloc(size, flags | __GFP_NOWARN);
+- if (\(!x\|x == NULL\))
+-   x = vmalloc(size);
++ T x = kvmalloc(size, flags);
+|
+- if (\(size <= E1\|size < E1\|size == E1\|size > E1\))
+-E = kzalloc(size, flags);
+- else
+-E = vzalloc(size);
++ E = kvzalloc(size, flags);
+|
+- E = kzalloc(size, flags | __GFP_NOWARN);
+- if (\(!E\|E == NULL\))
+-   E = vzalloc(size);
++ E = kvzalloc(size, flags);
+|
+- T x = kzalloc(size, flags | __GFP_NOWARN);
+- if (\(!x\|x == NULL\))
+-   x = vzalloc(size);
++ T x = kvzalloc(size, flags);
+|
+- if (\(size <= E1\|size < E1\|size == E1\|size > E1\))
+-E = kmalloc_node(size, flags, node);
+- else
+-E = vmalloc_node(size, node);
++ E = kvmalloc_node(size, flags, node);
+|
+- E = kmalloc_node(size, flags | __GFP_NOWARN, node);
+- if (\(!E\|E == NULL\))
+-   E = vmalloc_node(size, node);
++ E = kvmalloc_node(size, flags, node);
+|
+- T x = kmalloc_node(size, flags | __GFP_NOWARN, node);
+- if (\(!x\|x == NULL\))
+-   x = vmalloc_node(size, node);
++ T x = kvmalloc_node(size, flags, node);
+|
+- if (\(size <= E1\|size < E1\|size == E1\|size > E1\))
+-E = kvzalloc_node(size, flags, node);
+- else
+-E = vzalloc_node(size, node);
++ E = kvzalloc_node(size, flags, node);
+|
+- E = kvzalloc_node(size, flags | __GFP_NOWARN, node);
+- if (\(!E\|E == NULL\))
+-   E = vzalloc_node(size, node);
++ E = kvzalloc_node(size, flags, node);
+|
+- T x = kvzalloc_node(size, flags | __GFP_NOWARN, node);
+- if (\(!x\|x == NULL\))
+-   x = vzalloc_node(size, node);
++ T x = kvzalloc_node(size, flags, node);
+)
+
+@script: python depends on report@
+p << opportunity.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING: opportunity for kvmalloc")
+
+@script: python depends on org@
+p << opportunity.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING: opportunity for kvmalloc")
-- 
2.26.2

[PATCH v2] scsi: libcxgbi: use kvzalloc instead of opencoded kzalloc/vzalloc

[Denis Efremov]

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 = >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)
-- 
2.26.2

Re: [PATCH] scsi: libcxgbi: use kvzalloc instead of opencoded kzalloc/vzalloc

[Denis Efremov]

On 8/1/20 11:10 AM, Joe Perches wrote:
> On Sat, 2020-08-01 at 10:51 +0300, Denis Efremov wrote:
>> On 8/1/20 1:24 AM, Joe Perches wrote:
>>> On Sat, 2020-08-01 at 01:10 +0300, Denis Efremov wrote:
>>>> On 8/1/20 12:58 AM, Joe Perches wrote:
>>>>> On Sat, 2020-08-01 at 00:55 +0300, Denis Efremov wrote:
>>>>>> Remove cxgbi_alloc_big_mem(), cxgbi_free_big_mem() functions
>>>>>> and use kvzalloc/kvfree instead.
>>>>>
>>>>> Sensible, thanks.
>>>>>
>>>>>> diff --git 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 = >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);
>>>>>
>>>>> missing __GFP_NOWARN
>>>>>
>>>>
>>>> kvmalloc_node adds __GFP_NOWARN internally to kmalloc call
>>>> https://elixir.bootlin.com/linux/v5.8-rc4/source/mm/util.c#L568
>>>
>>> Only when there's a fallback, and the fallback does not.
>> Sorry, Joe, I don't understand why do we need to add __GFP_NOWARN here.
> 
> Hi.
> 
> The reason to add __GFP_NOWARN is so you don't get a
> dump_stack() as there's an existing error message
> output below this when OOM.
> 
> You should either remove the error message as it just
> effectively duplicates the dump_stack or add __GFP_NOWARN.

Now I see, thanks! I will send v2.

Regards,
Denis

Re: [PATCH] gfs2: Use kvmalloc instead of opencoded kmalloc/vmalloc

[Denis Efremov]

Please, skip this patch. I missed that kvmalloc checks (flags & GFP_KERNEL) == 
GFP_KERNEL
before calling vmalloc.

P.S.: previous mail was filtered because of html tags.

Thanks,
Denis

On 8/1/20 12:28 AM, Denis Efremov wrote:
> Use kvmalloc instead of opencoded kmalloc/vmalloc condition.
> 
> Signed-off-by: Denis Efremov 
> ---
>  fs/gfs2/dir.c   | 23 ---
>  fs/gfs2/quota.c |  5 +
>  2 files changed, 5 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
> index c0f2875c946c..5d2a708fae9c 100644
> --- a/fs/gfs2/dir.c
> +++ b/fs/gfs2/dir.c
> @@ -352,9 +352,7 @@ static __be64 *gfs2_dir_get_hash_table(struct gfs2_inode 
> *ip)
>   return ERR_PTR(-EIO);
>   }
>  
> - hc = kmalloc(hsize, GFP_NOFS | __GFP_NOWARN);
> - if (hc == NULL)
> - hc = __vmalloc(hsize, GFP_NOFS);
> + hc = kvmalloc(hsize, GFP_NOFS);
>  
>   if (hc == NULL)
>   return ERR_PTR(-ENOMEM);
> @@ -1320,18 +1318,6 @@ static int do_filldir_main(struct gfs2_inode *dip, 
> struct dir_context *ctx,
>   return 0;
>  }
>  
> -static void *gfs2_alloc_sort_buffer(unsigned size)
> -{
> - void *ptr = NULL;
> -
> - if (size < KMALLOC_MAX_SIZE)
> - ptr = kmalloc(size, GFP_NOFS | __GFP_NOWARN);
> - if (!ptr)
> - ptr = __vmalloc(size, GFP_NOFS);
> - return ptr;
> -}
> -
> -
>  static int gfs2_set_cookies(struct gfs2_sbd *sdp, struct buffer_head *bh,
>   unsigned leaf_nr, struct gfs2_dirent **darr,
>   unsigned entries)
> @@ -1409,7 +1395,8 @@ static int gfs2_dir_read_leaf(struct inode *inode, 
> struct dir_context *ctx,
>* 99 is the maximum number of entries that can fit in a single
>* leaf block.
>*/
> - larr = gfs2_alloc_sort_buffer((leaves + entries + 99) * sizeof(void *));
> + larr = kvmalloc_array(leaves + entries + 99,
> +   sizeof(void *), GFP_NOFS);
>   if (!larr)
>   goto out;
>   darr = (struct gfs2_dirent **)(larr + leaves);
> @@ -1985,9 +1972,7 @@ static int leaf_dealloc(struct gfs2_inode *dip, u32 
> index, u32 len,
>  
>   memset(, 0, sizeof(struct gfs2_rgrp_list));
>  
> - ht = kzalloc(size, GFP_NOFS | __GFP_NOWARN);
> - if (ht == NULL)
> - ht = __vmalloc(size, GFP_NOFS | __GFP_NOWARN | __GFP_ZERO);
> + ht = kvzalloc(size, GFP_NOFS);
>   if (!ht)
>   return -ENOMEM;
>  
> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> index 4b67d47a7e00..204b34f38e5c 100644
> --- a/fs/gfs2/quota.c
> +++ b/fs/gfs2/quota.c
> @@ -1362,10 +1362,7 @@ int gfs2_quota_init(struct gfs2_sbd *sdp)
>   bm_size = DIV_ROUND_UP(sdp->sd_quota_slots, 8 * sizeof(unsigned long));
>   bm_size *= sizeof(unsigned long);
>   error = -ENOMEM;
> - sdp->sd_quota_bitmap = kzalloc(bm_size, GFP_NOFS | __GFP_NOWARN);
> - if (sdp->sd_quota_bitmap == NULL)
> - sdp->sd_quota_bitmap = __vmalloc(bm_size, GFP_NOFS |
> -  __GFP_ZERO);
> + sdp->sd_quota_bitmap = kvzalloc(bm_size, GFP_NOFS);
>   if (!sdp->sd_quota_bitmap)
>   return error;
>  
> 

Re: [PATCH] scsi: libcxgbi: use kvzalloc instead of opencoded kzalloc/vzalloc

[Denis Efremov]

On 8/1/20 10:51 AM, Denis Efremov wrote:
> 
> 
> On 8/1/20 1:24 AM, Joe Perches wrote:
>> On Sat, 2020-08-01 at 01:10 +0300, Denis Efremov wrote:
>>>
>>> On 8/1/20 12:58 AM, Joe Perches wrote:
>>>> On Sat, 2020-08-01 at 00:55 +0300, Denis Efremov wrote:
>>>>> Remove cxgbi_alloc_big_mem(), cxgbi_free_big_mem() functions
>>>>> and use kvzalloc/kvfree instead.
>>>>
>>>> Sensible, thanks.
>>>>
>>>>> diff --git 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 = >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);
>>>>
>>>> missing __GFP_NOWARN
>>>>
>>>
>>> kvmalloc_node adds __GFP_NOWARN internally to kmalloc call
>>> https://elixir.bootlin.com/linux/v5.8-rc4/source/mm/util.c#L568
>>
>> Only when there's a fallback, and the fallback does not.
>>
> 
> Sorry, Joe, I don't understand why do we need to add __GFP_NOWARN here.
> 
> We use GFP_KERNEL for allocation. cxgbi_alloc_big_mem adds __GFP_NOWARN only 
> to kzalloc().
> kvzalloc is: kvmalloc(size, flags | __GFP_ZERO)
> kvmalloc is: kvmalloc_node(size, flags, NUMA_NO_NODE)
> kvmalloc_node: 
>   if ((flags & GFP_KERNEL) != GFP_KERNEL)   // false, flags == 
> GFP_KERNEL|__GFP_ZERO
>   return kmalloc_node(size, flags, node);
> 
>   if (size > PAGE_SIZE) {
>   kmalloc_flags |= __GFP_NOWARN;// add __GFP_NOWARN 
> for "big" allocations
> 
>   if (!(kmalloc_flags & __GFP_RETRY_MAYFAIL))
>   kmalloc_flags |= __GFP_NORETRY;
>   }
> 
>   // kmalloc_flags == GFP_KERNEL|__GFP_ZERO|__GFP_NOWARN if size > 
> PAGE_SIZE
>   ret = kmalloc_node(size, kmalloc_flags, node);
> 
>   if (ret || size <= PAGE_SIZE)
>   return ret;
> 
>   // flags == GFP_KERNEL|__GFP_ZERO
>   return __vmalloc_node(size, 1, flags, node,
>   __builtin_return_address(0));
> 
> So, to my understanding the difference is only that cxgbi_alloc_big_mem adds 
> __GFP_NOWARN
> to kzalloc unconditionally, kvzalloc adds __GFP_NOWARN to kmalloc_node if 
> size > PAGE_SIZE.
> We use: max_conn * sizeof(struct cxgbi_sock *) in allocation. max_conn can be 
> either
> CXGBI_MAX_CONN or CXGB4I_MAX_CONN. CXGBI_MAX_CONN == 16384, CXGB4I_MAX_CONN 
> == 16384.
> Thus the allocation is bigger than PAGE_SIZE and kvmalloc_node adds 
> __GFP_NOWARN to the
> kmalloc_node call. Maybe I missed something?
> 

Of course max_conn can be smaller than CXGBI_MAX_CONN and CXGB4I_MAX_CONN and 
size < PAGE_SIZE.
However, it's by design not to warn on "small" allocations in kvmalloc_node and 
it will not even
try to call __vmalloc_node if "small" allocation will fail with kmalloc_node. I 
think that this
behavior fits the cxgbi_device_portmap_create() context call.

Re: [PATCH] scsi: libcxgbi: use kvzalloc instead of opencoded kzalloc/vzalloc

[Denis Efremov]

On 8/1/20 1:24 AM, Joe Perches wrote:
> On Sat, 2020-08-01 at 01:10 +0300, Denis Efremov wrote:
>>
>> On 8/1/20 12:58 AM, Joe Perches wrote:
>>> On Sat, 2020-08-01 at 00:55 +0300, Denis Efremov wrote:
>>>> Remove cxgbi_alloc_big_mem(), cxgbi_free_big_mem() functions
>>>> and use kvzalloc/kvfree instead.
>>>
>>> Sensible, thanks.
>>>
>>>> diff --git 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 = >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);
>>>
>>> missing __GFP_NOWARN
>>>
>>
>> kvmalloc_node adds __GFP_NOWARN internally to kmalloc call
>> https://elixir.bootlin.com/linux/v5.8-rc4/source/mm/util.c#L568
> 
> Only when there's a fallback, and the fallback does not.
> 

Sorry, Joe, I don't understand why do we need to add __GFP_NOWARN here.

We use GFP_KERNEL for allocation. cxgbi_alloc_big_mem adds __GFP_NOWARN only to 
kzalloc().
kvzalloc is: kvmalloc(size, flags | __GFP_ZERO)
kvmalloc is: kvmalloc_node(size, flags, NUMA_NO_NODE)
kvmalloc_node: 
if ((flags & GFP_KERNEL) != GFP_KERNEL)   // false, flags == 
GFP_KERNEL|__GFP_ZERO
return kmalloc_node(size, flags, node);

if (size > PAGE_SIZE) {
kmalloc_flags |= __GFP_NOWARN;// add __GFP_NOWARN 
for "big" allocations

if (!(kmalloc_flags & __GFP_RETRY_MAYFAIL))
kmalloc_flags |= __GFP_NORETRY;
}

// kmalloc_flags == GFP_KERNEL|__GFP_ZERO|__GFP_NOWARN if size > 
PAGE_SIZE
ret = kmalloc_node(size, kmalloc_flags, node);

if (ret || size <= PAGE_SIZE)
return ret;

// flags == GFP_KERNEL|__GFP_ZERO
return __vmalloc_node(size, 1, flags, node,
__builtin_return_address(0));

So, to my understanding the difference is only that cxgbi_alloc_big_mem adds 
__GFP_NOWARN
to kzalloc unconditionally, kvzalloc adds __GFP_NOWARN to kmalloc_node if size 
> PAGE_SIZE.
We use: max_conn * sizeof(struct cxgbi_sock *) in allocation. max_conn can be 
either
CXGBI_MAX_CONN or CXGB4I_MAX_CONN. CXGBI_MAX_CONN == 16384, CXGB4I_MAX_CONN == 
16384.
Thus the allocation is bigger than PAGE_SIZE and kvmalloc_node adds 
__GFP_NOWARN to the
kmalloc_node call. Maybe I missed something?

Thanks,
Denis

Re: [PATCH] scsi: libcxgbi: use kvzalloc instead of opencoded kzalloc/vzalloc

[Denis Efremov]

On 8/1/20 12:58 AM, Joe Perches wrote:
> On Sat, 2020-08-01 at 00:55 +0300, Denis Efremov wrote:
>> Remove cxgbi_alloc_big_mem(), cxgbi_free_big_mem() functions
>> and use kvzalloc/kvfree instead.
> 
> Sensible, thanks.
> 
>> diff --git 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 = >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);
> 
> missing __GFP_NOWARN
> 

kvmalloc_node adds __GFP_NOWARN internally to kmalloc call
https://elixir.bootlin.com/linux/v5.8-rc4/source/mm/util.c#L568

Thanks,
Denis

[PATCH] scsi: libcxgbi: use kvzalloc instead of opencoded kzalloc/vzalloc

[Denis Efremov]

Remove cxgbi_alloc_big_mem(), cxgbi_free_big_mem() functions
and use kvzalloc/kvfree instead.

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..a98b870eb1f3 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 = >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);
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)
-- 
2.26.2

[PATCH] gfs2: Use kvmalloc instead of opencoded kmalloc/vmalloc

[Denis Efremov]

Use kvmalloc instead of opencoded kmalloc/vmalloc condition.

Signed-off-by: Denis Efremov 
---
 fs/gfs2/dir.c   | 23 ---
 fs/gfs2/quota.c |  5 +
 2 files changed, 5 insertions(+), 23 deletions(-)

diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index c0f2875c946c..5d2a708fae9c 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -352,9 +352,7 @@ static __be64 *gfs2_dir_get_hash_table(struct gfs2_inode 
*ip)
return ERR_PTR(-EIO);
}
 
-   hc = kmalloc(hsize, GFP_NOFS | __GFP_NOWARN);
-   if (hc == NULL)
-   hc = __vmalloc(hsize, GFP_NOFS);
+   hc = kvmalloc(hsize, GFP_NOFS);
 
if (hc == NULL)
return ERR_PTR(-ENOMEM);
@@ -1320,18 +1318,6 @@ static int do_filldir_main(struct gfs2_inode *dip, 
struct dir_context *ctx,
return 0;
 }
 
-static void *gfs2_alloc_sort_buffer(unsigned size)
-{
-   void *ptr = NULL;
-
-   if (size < KMALLOC_MAX_SIZE)
-   ptr = kmalloc(size, GFP_NOFS | __GFP_NOWARN);
-   if (!ptr)
-   ptr = __vmalloc(size, GFP_NOFS);
-   return ptr;
-}
-
-
 static int gfs2_set_cookies(struct gfs2_sbd *sdp, struct buffer_head *bh,
unsigned leaf_nr, struct gfs2_dirent **darr,
unsigned entries)
@@ -1409,7 +1395,8 @@ static int gfs2_dir_read_leaf(struct inode *inode, struct 
dir_context *ctx,
 * 99 is the maximum number of entries that can fit in a single
 * leaf block.
 */
-   larr = gfs2_alloc_sort_buffer((leaves + entries + 99) * sizeof(void *));
+   larr = kvmalloc_array(leaves + entries + 99,
+ sizeof(void *), GFP_NOFS);
if (!larr)
goto out;
darr = (struct gfs2_dirent **)(larr + leaves);
@@ -1985,9 +1972,7 @@ static int leaf_dealloc(struct gfs2_inode *dip, u32 
index, u32 len,
 
memset(, 0, sizeof(struct gfs2_rgrp_list));
 
-   ht = kzalloc(size, GFP_NOFS | __GFP_NOWARN);
-   if (ht == NULL)
-   ht = __vmalloc(size, GFP_NOFS | __GFP_NOWARN | __GFP_ZERO);
+   ht = kvzalloc(size, GFP_NOFS);
if (!ht)
return -ENOMEM;
 
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 4b67d47a7e00..204b34f38e5c 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -1362,10 +1362,7 @@ int gfs2_quota_init(struct gfs2_sbd *sdp)
bm_size = DIV_ROUND_UP(sdp->sd_quota_slots, 8 * sizeof(unsigned long));
bm_size *= sizeof(unsigned long);
error = -ENOMEM;
-   sdp->sd_quota_bitmap = kzalloc(bm_size, GFP_NOFS | __GFP_NOWARN);
-   if (sdp->sd_quota_bitmap == NULL)
-   sdp->sd_quota_bitmap = __vmalloc(bm_size, GFP_NOFS |
-__GFP_ZERO);
+   sdp->sd_quota_bitmap = kvzalloc(bm_size, GFP_NOFS);
if (!sdp->sd_quota_bitmap)
return error;
 
-- 
2.26.2

[PATCH v6] coccinelle: api: add kvfree script

[Denis Efremov]

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

 scripts/coccinelle/api/kvfree.cocci | 182 
 1 file changed, 182 insertions(+)
 create mode 100644 scripts/coccinelle/api/kvfree.cocci

diff --git a/scripts/coccinelle/api/kvfree.cocci 
b/scripts/coccinelle/api/kvfree.cocci
new file mode 100644
index ..f43cda5b0d5b
--- /dev/null
+++ b/scripts/coccinelle/api/kvfree.cocci
@@ -0,0 +1,182 @@
+// 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: Medium
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual patch
+virtual report
+virtual org
+virtual context
+
+@initialize:python@
+@@
+# low-level memory api
+filter = frozenset(['__vmalloc_area_node'])
+
+def relevant(p):
+return not (filter & {el.current_element for el in p})
+
+@choice@
+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\|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
+...
+  }
+)
+
+@vfree depends on !patch@
+expression E;
+position k != choice.kok;
+position p;
+@@
+
+* E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|
+*   kzalloc_node\|kmalloc_array\|kmalloc_array_node\|
+*   kcalloc_node\)(...)@k
+  ... 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)@p
+
+@pvfree depends on patch exists@
+expression E;
+position k != choice.kok;
+@@
+
+  E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|
+kzalloc_node\|kmalloc_array\|kmalloc_array_node\|
+kcalloc_node\)(...)@k
+  ... 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)
++ kfree(E)
+
+@kfree depends on !patch@
+expression E;
+position v != choice.vok;
+position p;
+@@
+
+* E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|
+*   vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|
+*   __vmalloc_node_range\|__vmalloc_node\)(...)@v
+  ... when != !is_vmalloc_addr(E)
+  when any
+* \(kfree\|kzfree\|kvfree\)(E)@p
+
+@pkfree depends on patch exists@
+expression E;
+position v != choice.vok;
+@@
+
+  E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|
+vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|
+__vmalloc_node_range\|__vmalloc_node\)(...)@v
+  ... when != !is_vmalloc_addr(E)
+  when any
+- \(kfree\|kvfree\)(E)
++ vfree(E)
+
+@kvfree depends on !patch@
+expression E;
+position p, k;
+@@
+
+* E = \(kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|
+*   kvmalloc_array\)(...)@k
+  ... when != is_vmalloc_addr(E)
+  when any
+* \(kfree\|kzfree\|vfree\|vfree_atomic\)(E)@p
+
+@pkvfree depends on patch exists@
+expression E;
+@@
+
+  E = \(kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|
+kvmalloc_array\)(...)
+  ... when != is_vmalloc_addr(E)
+  when any
+- \(kfree\|vfree\)(E)
++ kvfree(E)
+
+@script: python depends on report@
+k << vfree.

[PATCH v5] coccinelle: api: add kvfree script

[Denis Efremov]

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

 scripts/coccinelle/api/kvfree.cocci | 182 
 1 file changed, 182 insertions(+)
 create mode 100644 scripts/coccinelle/api/kvfree.cocci

diff --git a/scripts/coccinelle/api/kvfree.cocci 
b/scripts/coccinelle/api/kvfree.cocci
new file mode 100644
index ..e83b1ebbad7e
--- /dev/null
+++ b/scripts/coccinelle/api/kvfree.cocci
@@ -0,0 +1,182 @@
+// 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: Medium
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual patch
+virtual report
+virtual org
+virtual context
+
+@initialize:python@
+@@
+# low-level memory api
+filter = frozenset(['__vmalloc_area_node'])
+
+def relevant(p):
+return not (filter & {el.current_element for el in p})
+
+@choice@
+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\)(...)
+  ... when != E = E1
+  when any
+  if (\(!E\|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
+...
+  }
+)
+
+@vfree depends on !patch@
+expression E;
+position k != choice.kok;
+position p;
+@@
+
+* E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|
+kzalloc_node\|kmalloc_array\|kmalloc_array_node\|
+kcalloc_node\)(...)@k
+  ... 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)@p
+
+@pvfree depends on patch exists@
+expression E;
+position k != choice.kok;
+@@
+
+  E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|
+kzalloc_node\|kmalloc_array\|kmalloc_array_node\|
+kcalloc_node\)(...)@k
+  ... 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)
++ kfree(E)
+
+@kfree depends on !patch@
+expression E;
+position v != choice.vok;
+position p;
+@@
+
+* E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|
+vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|
+__vmalloc_node_range\|__vmalloc_node\)(...)@v
+  ... when != !is_vmalloc_addr(E)
+  when any
+* \(kfree\|kzfree\|kvfree\)(E)@p
+
+@pkfree depends on patch exists@
+expression E;
+position v != choice.vok;
+@@
+
+  E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|
+vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|
+__vmalloc_node_range\|__vmalloc_node\)(...)@v
+  ... when != !is_vmalloc_addr(E)
+  when any
+- \(kfree\|kvfree\)(E)
++ vfree(E)
+
+@kvfree depends on !patch@
+expression E;
+position p, k;
+@@
+
+* E = \(kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|
+kvmalloc_array\)(...)@k
+  ... when != is_vmalloc_addr(E)
+  when any
+* \(kfree\|kzfree\|vfree\|vfree_atomic\)(E)@p
+
+@pkvfree depends on patch exists@
+expression E;
+@@
+
+  E = \(kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|
+kvmalloc_array\)(...)
+  ... when != is_vmalloc_addr(E)
+  when any
+- \(kfree\|vfree\)(E)
++ kvfree(E)
+
+@script: python depends on report@
+k << vfree.k;
+p << vfree.p;
+@@
+
+msg = "WARNING: kmalloc is used to allocat

Re: [PATCH v4] coccinelle: api: add kvfree script

[Denis Efremov]

> With the current patch mode, I got some changes in a recent linux-next.
> Have you sent patches for these issues?

For mellanox, I've sent these patches:
https://lkml.org/lkml/2020/6/5/901
https://lkml.org/lkml/2020/6/1/713
They were accepted.

I see two new places in mellanox driver in linux-next. It looks like this
is new code that is not yet merged to the linux master branch.

diff -u -p a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c 
b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
--- 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_pri
fs_prot->miss_rule = miss_rule;

out:
-   kfree(flow_group_in);
-   kfree(spec);
+   kvfree(flow_group_in);
+   kvfree(spec);
return err;
}

diff -u -p a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c 
b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c
--- 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(st
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;
}

I will send the fixes when the code will be merged to the linux master branch.
Maybe it will be fixed already in net-next at that time.

> 
> Do the checks for the opportunities for kvmalloc really belong in this
> rule?  That issue is not mentioned in the commit log or the description of
> the semantic patch.

I added this at the last moment. It was easy enough to add it based on existing
patterns. I will add description for this warnings. Or do you want me to single
out this warning to a separate rule?


Regards,
Denis

[PATCH v4] coccinelle: api: add kvfree script

[Denis Efremov]

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

 scripts/coccinelle/api/kvfree.cocci | 227 
 1 file changed, 227 insertions(+)
 create mode 100644 scripts/coccinelle/api/kvfree.cocci

diff --git a/scripts/coccinelle/api/kvfree.cocci 
b/scripts/coccinelle/api/kvfree.cocci
new file mode 100644
index ..d73578c5549c
--- /dev/null
+++ b/scripts/coccinelle/api/kvfree.cocci
@@ -0,0 +1,227 @@
+// 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: Medium
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual patch
+virtual report
+virtual org
+virtual context
+
+@initialize:python@
+@@
+# low-level memory api
+filter = frozenset(['__vmalloc_area_node'])
+
+def relevant(p):
+return not (filter & {el.current_element for el in p})
+
+@choice@
+expression E, E1;
+position kok, vok;
+@@
+
+(
+  if (...) {
+...
+E = \(kmalloc@kok\|kzalloc@kok\|krealloc@kok\|kcalloc@kok\|
+  kmalloc_node@kok\|kzalloc_node@kok\|kmalloc_array@kok\|
+  kmalloc_array_node@kok\|kcalloc_node@kok\)(...)
+...
+  } else {
+...
+E = \(vmalloc@vok\|vzalloc@vok\|vmalloc_user@vok\|vmalloc_node@vok\|
+  vzalloc_node@vok\|vmalloc_exec@vok\|vmalloc_32@vok\|
+  vmalloc_32_user@vok\|__vmalloc@vok\|__vmalloc_node_range@vok\|
+  __vmalloc_node@vok\)(...)
+...
+  }
+|
+  E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(...)
+  ... when != E = E1
+  when any
+  if (\(!E\|E == NULL\)) {
+...
+E = \(vmalloc@vok\|vzalloc@vok\|vmalloc_user@vok\|vmalloc_node@vok\|
+  vzalloc_node@vok\|vmalloc_exec@vok\|vmalloc_32@vok\|
+  vmalloc_32_user@vok\|__vmalloc@vok\|__vmalloc_node_range@vok\|
+  __vmalloc_node@vok\)(...)
+...
+  }
+)
+
+@opportunity depends on !patch@
+expression E, E1, size;
+position p : script:python() { relevant(p) };
+@@
+
+(
+* if (\(size <= E1\|size < E1\|size = E1\|size > E1\) || ...)@p {
+...
+E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+  kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(..., size, ...)
+...
+  } else {
+...
+E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|
+  vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|
+  __vmalloc_node_range\|__vmalloc_node\)(..., size, ...)
+...
+  }
+|
+  E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(..., size, ...)
+  ... when != E = E1
+  when != size = E1
+  when any
+* if (\(!E\|E == NULL\))@p {
+...
+E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|
+  vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|
+  __vmalloc_node_range\|__vmalloc_node\)(..., size, ...)
+...
+  }
+)
+
+@vfree depends on !patch@
+expression E;
+position k != choice.kok;
+position p;
+@@
+
+* E = \(kmalloc@k\|kzalloc@k\|krealloc@k\|kcalloc@k\|kmalloc_node@k\|
+kzalloc_node@k\|kmalloc_array@k\|kmalloc_array_node@k\|
+kcalloc_node@k\)(...)
+  ... 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)@p
+
+@pvfree depends on patch exists@
+expression E;
+position k != choice.kok;
+@@
+
+  E = \(kmalloc@k\|kzalloc@k\|krealloc@k\|kcalloc@k\|kmalloc_node@k\|
+kzalloc_node@k\|kmalloc_array@k\|kmalloc_array_node@k\|
+kcalloc_node@k\)(...)
+  ... 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)
++ kfree(E)
+
+@kfree depends on !patch@
+expression E;
+position v != choice.vok;
+position p;
+@@
+
+* E = \(vmalloc@v\|vzalloc@v\|vmalloc_user@v\|vmalloc_node@v\|vzalloc_node@v\|
+vmalloc_exec@v\|vmalloc_32@v\|vmalloc_32_user@v\|__vmalloc@v\|
+__vmalloc_node_range@v\|__vmalloc_node@v\)(...)
+ 

Re: [PATCH v3] coccinelle: api: add kvfree script

[Denis Efremov]

> +
> +@script: python depends on org@
> +v << kfree.v;
> +p << kfree.p;
> +@@
> +
> +msg = "WARNING: vmalloc is used to allocate this memory at line %s" % 
> (v[0].line)
> +coccilib.org.print_todo(p[0],

Just noticed this error. I will resend the patch in 5mins.

Regards,
Denis

[PATCH v3] coccinelle: api: add kvfree script

[Denis Efremov]

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

 scripts/coccinelle/api/kvfree.cocci | 227 
 1 file changed, 227 insertions(+)
 create mode 100644 scripts/coccinelle/api/kvfree.cocci

diff --git a/scripts/coccinelle/api/kvfree.cocci 
b/scripts/coccinelle/api/kvfree.cocci
new file mode 100644
index ..7c396daeacad
--- /dev/null
+++ b/scripts/coccinelle/api/kvfree.cocci
@@ -0,0 +1,227 @@
+// 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: Medium
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual patch
+virtual report
+virtual org
+virtual context
+
+@initialize:python@
+@@
+# low-level memory api
+filter = frozenset(['__vmalloc_area_node'])
+
+def relevant(p):
+return not (filter & {el.current_element for el in p})
+
+@choice@
+expression E, E1;
+position kok, vok;
+@@
+
+(
+  if (...) {
+...
+E = \(kmalloc@kok\|kzalloc@kok\|krealloc@kok\|kcalloc@kok\|
+  kmalloc_node@kok\|kzalloc_node@kok\|kmalloc_array@kok\|
+  kmalloc_array_node@kok\|kcalloc_node@kok\)(...)
+...
+  } else {
+...
+E = \(vmalloc@vok\|vzalloc@vok\|vmalloc_user@vok\|vmalloc_node@vok\|
+  vzalloc_node@vok\|vmalloc_exec@vok\|vmalloc_32@vok\|
+  vmalloc_32_user@vok\|__vmalloc@vok\|__vmalloc_node_range@vok\|
+  __vmalloc_node@vok\)(...)
+...
+  }
+|
+  E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(...)
+  ... when != E = E1
+  when any
+  if (\(!E\|E == NULL\)) {
+...
+E = \(vmalloc@vok\|vzalloc@vok\|vmalloc_user@vok\|vmalloc_node@vok\|
+  vzalloc_node@vok\|vmalloc_exec@vok\|vmalloc_32@vok\|
+  vmalloc_32_user@vok\|__vmalloc@vok\|__vmalloc_node_range@vok\|
+  __vmalloc_node@vok\)(...)
+...
+  }
+)
+
+@opportunity depends on !patch@
+expression E, E1, size;
+position p : script:python() { relevant(p) };
+@@
+
+(
+* if (\(size <= E1\|size < E1\|size = E1\|size > E1\) || ...)@p {
+...
+E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+  kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(..., size, ...)
+...
+  } else {
+...
+E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|
+  vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|
+  __vmalloc_node_range\|__vmalloc_node\)(..., size, ...)
+...
+  }
+|
+  E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(..., size, ...)
+  ... when != E = E1
+  when != size = E1
+  when any
+* if (\(!E\|E == NULL\))@p {
+...
+E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|
+  vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|
+  __vmalloc_node_range\|__vmalloc_node\)(..., size, ...)
+...
+  }
+)
+
+@vfree depends on !patch@
+expression E;
+position k != choice.kok;
+position p;
+@@
+
+* E = \(kmalloc@k\|kzalloc@k\|krealloc@k\|kcalloc@k\|kmalloc_node@k\|
+kzalloc_node@k\|kmalloc_array@k\|kmalloc_array_node@k\|
+kcalloc_node@k\)(...)
+  ... 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)@p
+
+@pvfree depends on patch exists@
+expression E;
+position k != choice.kok;
+@@
+
+  E = \(kmalloc@k\|kzalloc@k\|krealloc@k\|kcalloc@k\|kmalloc_node@k\|
+kzalloc_node@k\|kmalloc_array@k\|kmalloc_array_node@k\|
+kcalloc_node@k\)(...)
+  ... 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)
++ kfree(E)
+
+@kfree depends on !patch@
+expression E;
+position v != choice.vok;
+position p;
+@@
+
+* E = \(vmalloc@v\|vzalloc@v\|vmalloc_user@v\|vmalloc_node@v\|vzalloc_node@v\|
+vmalloc_exec@v\|vmalloc_32@v\|vmalloc_32_user@v\|__vmalloc@v\|
+__vmalloc_node_range@v\|__vmalloc_node@v\)(...)
+  ... when != !is_vmalloc_addr(E)
+  when any
+* \(kfree\|kzfree\

Re: [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()

[Denis Efremov]

On 7/29/20 3:58 PM, Dan Carpenter wrote:
> Argh...  This isn't right still.  The "ptr" comes from raw_cmd_copyin()
> 
> ptr = kmalloc(sizeof(struct floppy_raw_cmd), GFP_KERNEL);
> 

copy_from_user overwrites the padding bytes:
ptr = kmalloc(sizeof(struct floppy_raw_cmd), GFP_KERNEL);
if (!ptr)
return -ENOMEM;
*rcmd = ptr;
ret = copy_from_user(ptr, param, sizeof(*ptr));

I think memcpy should be safe in this patch.

I've decided to dig a bit into the issue and to run some tests.
Here are my observations:

$ cat test.c

#include 
#include 

#define __user

struct floppy_raw_cmd {
unsigned int flags;
void __user *data;
char *kernel_data; /* location of data buffer in the kernel */
struct floppy_raw_cmd *next; /* used for chaining of raw cmd's 
  * within the kernel */
long length; /* in: length of dma transfer. out: remaining bytes */
long phys_length; /* physical length, if different from dma length */
int buffer_length; /* length of allocated buffer */

unsigned char rate;

#define FD_RAW_CMD_SIZE 16
#define FD_RAW_REPLY_SIZE 16
#define FD_RAW_CMD_FULLSIZE (FD_RAW_CMD_SIZE + 1 + FD_RAW_REPLY_SIZE)

unsigned char cmd_count;
union {
struct {
unsigned char cmd[FD_RAW_CMD_SIZE];
unsigned char reply_count;
unsigned char reply[FD_RAW_REPLY_SIZE];
};
unsigned char fullcmd[FD_RAW_CMD_FULLSIZE];
};
int track;
int resultcode;

int reserved1;
int reserved2;
};

void __attribute__((noinline)) stack_alloc()
{
struct floppy_raw_cmd stack;
memset(, 0xff, sizeof(struct floppy_raw_cmd));
asm volatile ("" ::: "memory");
}

int __attribute__((noinline)) test(struct floppy_raw_cmd *ptr)
{
struct floppy_raw_cmd cmd = *ptr;
int i;

for (i = 0; i < sizeof(struct floppy_raw_cmd); ++i) {
if (((char *))[i]) {
printf("leak[%d]\n", i);
return i;
}
}

return 0;
}

int main(int argc, char *argv[])
{
struct floppy_raw_cmd zero;

memset(, 0, sizeof(struct floppy_raw_cmd));
// For selfcheck uncomment:
// zero.resultcode = 1;
stack_alloc();
return test();
}

Next, I've prepared containers with gcc 4.8 5 6 7 8 9 10 versions with this
tool (https://github.com/a13xp0p0v/kernel-build-containers).

And checked for leaks on x86_64 with the script test.sh
$ cat test.sh
#!/bin/bash

for i in 4.8 5 6 7 8 9 10
do
./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc test.c; ./a.out'
./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc -O2 test.c; 
./a.out'
./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc -O3 test.c; 
./a.out'
done

No leaks reported. Is it really possible this this kind of init, i.e. cmd = 
*ptr?

https://lwn.net/Articles/417989/ (December 1, 2010).
GCC 4.9.4 released [2016-08-03]
Maybe this behavior changed.

https://www.nccgroup.com/us/about-us/newsroom-and-events/blog/2019/october/padding-the-struct-how-a-compiler-optimization-can-disclose-stack-memory/
Reports for >= 4.7, < 8.0 version. But I can't find a word about this
kind of inits: cmd = *ptr.

Thanks,
Denis

Re: [Linux-kernel-mentees] [PATCH] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()

[Denis Efremov]

On 7/28/20 5:19 PM, Peilin Ye wrote:
> raw_cmd_copyout() is potentially copying uninitialized kernel stack memory
> since it is initializing `cmd` by assignment, which may cause the compiler
> to leave uninitialized holes in this structure. Fix it by using memcpy()
> instead.
> 
> Cc: sta...@vger.kernel.org
> Fixes: 2145e15e0557 ("floppy: don't write kernel-only members to FDRAWCMD 
> ioctl output")

Nitpick, I would say this fix is not related to commit 2145e15e0557.

> Suggested-by: Dan Carpenter 
> Suggested-by: Arnd Bergmann 
> Signed-off-by: Peilin Ye 

Re: [Linux-kernel-mentees] [PATCH] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()

[Denis Efremov]

Hi,

On 7/28/20 5:19 PM, Peilin Ye wrote:
> raw_cmd_copyout() is potentially copying uninitialized kernel stack memory
> since it is initializing `cmd` by assignment, which may cause the compiler
> to leave uninitialized holes in this structure. Fix it by using memcpy()
> instead.
> 
> Cc: sta...@vger.kernel.org
> Fixes: 2145e15e0557 ("floppy: don't write kernel-only members to FDRAWCMD 
> ioctl output")
> Suggested-by: Dan Carpenter 
> Suggested-by: Arnd Bergmann 
> Signed-off-by: Peilin Ye 

Reviewed-by: Denis Efremov 

ptr comes from raw_cmd_copyin and it should be ok to use memcpy.

Jens, could you please take this one to your 5.9 branch?


> ---
> $ pahole -C "floppy_raw_cmd" drivers/block/floppy.o
> struct floppy_raw_cmd {
>   unsigned int   flags;/* 0 4 */
> 
>   /* XXX 4 bytes hole, try to pack */
> 
>   void * data; /* 8 8 */
>   char * kernel_data;  /*16 8 */
>   struct floppy_raw_cmd *next; /*24 8 */
>   long int   length;   /*32 8 */
>   long int   phys_length;  /*40 8 */
>   intbuffer_length;/*48 4 */
>   unsigned char  rate; /*52 1 */
>   unsigned char  cmd_count;/*53 1 */
>   union {
>   struct {
>   unsigned char cmd[16];   /*5416 */
>   /* --- cacheline 1 boundary (64 bytes) was 6 bytes ago 
> --- */
>   unsigned char reply_count;   /*70 1 */
>   unsigned char reply[16]; /*7116 */
>   };   /*5433 */
>   unsigned char  fullcmd[33];  /*5433 */
>   };   /*5433 */
> 
>   /* XXX 1 byte hole, try to pack */
> 
>   /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
>   inttrack;/*88 4 */
>   intresultcode;   /*92 4 */
>   intreserved1;/*96 4 */
>   intreserved2;/*   100 4 */
> 
>   /* size: 104, cachelines: 2, members: 14 */
>   /* sum members: 99, holes: 2, sum holes: 5 */
>   /* last cacheline: 40 bytes */
> };
> 

It would be nice to add lkml links with discussion on the issue or
https://www.nccgroup.com/us/about-us/newsroom-and-events/blog/2019/october/padding-the-struct-how-a-compiler-optimization-can-disclose-stack-memory/
in addition to pahole output.

>  drivers/block/floppy.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 09079aee8dc4..b8ea98f7a9cb 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -3126,7 +3126,9 @@ static int raw_cmd_copyout(int cmd, void __user *param,
>   int ret;
>  
>   while (ptr) {
> - struct floppy_raw_cmd cmd = *ptr;
> + struct floppy_raw_cmd cmd;
> +
> + memcpy(, ptr, sizeof(cmd))> cmd.next = NULL;
>   cmd.kernel_data = NULL;
>   ret = copy_to_user(param, , sizeof(cmd));
> 

Thanks,
Denis

[PATCH v3 1/3] coccinelle: api: extend memdup_user transformation with GFP_USER

[Denis Efremov]

Match GFP_USER and optional __GFP_NOWARN allocations with
memdup_user.cocci rule.
Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
memdup_user() from GFP_KERNEL to GFP_USER. In almost all cases it
is still a good idea to recommend memdup_user() for GFP_KERNEL
allocations. The motivation behind altering memdup_user() to GFP_USER:
https://lkml.org/lkml/2018/1/6/333

Signed-off-by: Denis Efremov 
---
 scripts/coccinelle/api/memdup_user.cocci | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/scripts/coccinelle/api/memdup_user.cocci 
b/scripts/coccinelle/api/memdup_user.cocci
index c809ab10bbce..0e29d41ecab8 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -20,7 +20,9 @@ expression from,to,size;
 identifier l1,l2;
 @@
 
--  to = \(kmalloc\|kzalloc\)(size,GFP_KERNEL);
+-  to = \(kmalloc\|kzalloc\)
+-  (size,\(GFP_KERNEL\|GFP_USER\|
+-\(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
 +  to = memdup_user(from,size);
if (
 -  to==NULL
@@ -43,7 +45,9 @@ position p;
 statement S1,S2;
 @@
 
-*  to = \(kmalloc@p\|kzalloc@p\)(size,GFP_KERNEL);
+*  to = \(kmalloc@p\|kzalloc@p\)
+   (size,\(GFP_KERNEL\|GFP_USER\|
+ \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
if (to==NULL || ...) S1
if (copy_from_user(to, from, size) != 0)
S2
-- 
2.26.2

[PATCH v3 2/3] coccinelle: api: extend memdup_user rule with vmemdup_user()

[Denis Efremov]

Add vmemdup_user() transformations to the memdup_user.cocci rule.
Commit 50fd2f298bef ("new primitive: vmemdup_user()") introduced
vmemdup_user(). The function uses kvmalloc with GPF_USER flag.

Signed-off-by: Denis Efremov 
---
 scripts/coccinelle/api/memdup_user.cocci | 45 
 1 file changed, 45 insertions(+)

diff --git a/scripts/coccinelle/api/memdup_user.cocci 
b/scripts/coccinelle/api/memdup_user.cocci
index 0e29d41ecab8..60027e21c5e6 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -39,6 +39,28 @@ identifier l1,l2;
 -...+>
 -  }
 
+@depends on patch@
+expression from,to,size;
+identifier l1,l2;
+@@
+
+-  to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
++  to = vmemdup_user(from,size);
+   if (
+-  to==NULL
++  IS_ERR(to)
+ || ...) {
+   <+... when != goto l1;
+-  -ENOMEM
++  PTR_ERR(to)
+   ...+>
+   }
+-  if (copy_from_user(to, from, size) != 0) {
+-<+... when != goto l2;
+--EFAULT
+-...+>
+-  }
+
 @r depends on !patch@
 expression from,to,size;
 position p;
@@ -52,6 +74,17 @@ statement S1,S2;
if (copy_from_user(to, from, size) != 0)
S2
 
+@rv depends on !patch@
+expression from,to,size;
+position p;
+statement S1,S2;
+@@
+
+*  to = \(kvmalloc@p\|kvzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
+   if (to==NULL || ...) S1
+   if (copy_from_user(to, from, size) != 0)
+   S2
+
 @script:python depends on org@
 p << r.p;
 @@
@@ -63,3 +96,15 @@ p << r.p;
 @@
 
 coccilib.report.print_report(p[0], "WARNING opportunity for memdup_user")
+
+@script:python depends on org@
+p << rv.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for vmemdup_user")
+
+@script:python depends on report@
+p << rv.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for vmemdup_user")
-- 
2.26.2

[PATCH v3 3/3] coccinelle: api: filter out memdup_user definitions

[Denis Efremov]

Don't match memdup_user/vmemdup_user.

Signed-off-by: Denis Efremov 
---
 scripts/coccinelle/api/memdup_user.cocci | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/scripts/coccinelle/api/memdup_user.cocci 
b/scripts/coccinelle/api/memdup_user.cocci
index 60027e21c5e6..e01e95108405 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -15,12 +15,20 @@ virtual context
 virtual org
 virtual report
 
+@initialize:python@
+@@
+filter = frozenset(['memdup_user', 'vmemdup_user'])
+
+def relevant(p):
+return not (filter & {el.current_element for el in p})
+
 @depends on patch@
 expression from,to,size;
 identifier l1,l2;
+position p : script:python() { relevant(p) };
 @@
 
--  to = \(kmalloc\|kzalloc\)
+-  to = \(kmalloc@p\|kzalloc@p\)
 -  (size,\(GFP_KERNEL\|GFP_USER\|
 -\(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
 +  to = memdup_user(from,size);
@@ -42,9 +50,10 @@ identifier l1,l2;
 @depends on patch@
 expression from,to,size;
 identifier l1,l2;
+position p : script:python() { relevant(p) };
 @@
 
--  to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
+-  to = \(kvmalloc@p\|kvzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
 +  to = vmemdup_user(from,size);
if (
 -  to==NULL
@@ -63,7 +72,7 @@ identifier l1,l2;
 
 @r depends on !patch@
 expression from,to,size;
-position p;
+position p : script:python() { relevant(p) };
 statement S1,S2;
 @@
 
@@ -76,7 +85,7 @@ statement S1,S2;
 
 @rv depends on !patch@
 expression from,to,size;
-position p;
+position p : script:python() { relevant(p) };
 statement S1,S2;
 @@
 
-- 
2.26.2

[PATCH v3 0/3] Update memdup_user.cocci

[Denis Efremov]

Add GFP_USER to the allocation flags and handle vmemdup_user().

Changes in v2:
 - memdup_user/vmemdup_user matching suppressed
 - PoC for selfcheck virtual rule
Changes in v3:
 - add missing '-' for patch rule in kmalloc/kzalloc call args
 - selfcheck rule dropped from patchset

Denis Efremov (3):
  coccinelle: api: extend memdup_user transformation with GFP_USER
  coccinelle: api: extend memdup_user rule with vmemdup_user()
  coccinelle: api: filter out memdup_user definitions

 scripts/coccinelle/api/memdup_user.cocci | 64 ++--
 1 file changed, 61 insertions(+), 3 deletions(-)

-- 
2.26.2

Re: [v2 1/4] coccinelle: api: extend memdup_user transformation with GFP_USER

[Denis Efremov]

Hi,

On 7/18/20 9:45 AM, Julia Lawall wrote:
> This on is indeed a problem.  I think it was not detected in testing,
> because in the current kernel the rule never applies.  But Denis, in
> 
> -  to = \(kmalloc\|kzalloc\)
> (size,\(GFP_KERNEL\|GFP_USER\|
>   \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
> 
> you do indeed need to put - in front of the second and third lines as
> well.

Thanks, Markus, Julia. I will send v3. Julia, is it ok with you, if I will
drop the last patch with "selfcheck" this time?

Regards,
Denis

Re: [PATCH v2 0/4] Update memdup_user.cocci

[Denis Efremov]

Ping?

On 6/8/20 6:00 PM, Denis Efremov wrote:
> Add GFP_USER to the allocation flags and handle vmemdup_user().
> The third patch supresses memdup_user(), vmemdup_user() functions
> detection. Last patch is a proof of concept for the rule selfchecking.
> Gives the ability to detect that an open-coded pattern in a function
> definition that we search for in the kernel sources changed.
> 
> Denis Efremov (4):
>   coccinelle: api: extend memdup_user transformation with GFP_USER
>   coccinelle: api: extend memdup_user rule with vmemdup_user()
>   coccinelle: api: filter out memdup_user definitions
>   coccinelle: api: add selfcheck for memdup_user rule
> 
>  scripts/coccinelle/api/memdup_user.cocci | 106 ++-
>  1 file changed, 103 insertions(+), 3 deletions(-)
> 

Re: [PATCH v2] coccinelle: api: add kvfree script

[Denis Efremov]

Ping?

[PATCH v4] coccinelle: api: add kzfree script

[Denis Efremov]

Check for memset()/memzero_explicit() followed by kfree()/vfree()/kvfree().

Signed-off-by: Denis Efremov 
---
Changes in v2:
 - memset_explicit() added
 - kvfree_sensitive() added
 - forall added to r1
 - ... between memset and kfree added
Changes in v3:
 - Explicit filter for definitions instead of !(file in "...") conditions
 - type T added to match casts
 - memzero_explicit() patterns fixed
 - additional rule "cond" added to filter false-positives
Changes in v4:
 - memset call fixed in rp_memset
 - @m added to rp_memset,rp_memzero rules

 scripts/coccinelle/api/kzfree.cocci | 101 
 1 file changed, 101 insertions(+)
 create mode 100644 scripts/coccinelle/api/kzfree.cocci

diff --git a/scripts/coccinelle/api/kzfree.cocci 
b/scripts/coccinelle/api/kzfree.cocci
new file mode 100644
index ..33625bd7cec9
--- /dev/null
+++ b/scripts/coccinelle/api/kzfree.cocci
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Use kzfree, kvfree_sensitive rather than memset or
+/// memzero_explicit followed by kfree
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: kzfree, kvfree_sensitive
+//
+
+virtual context
+virtual patch
+virtual org
+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'])
+
+def relevant(p):
+return not (filter & {el.current_element for el in p})
+
+@cond@
+position ok;
+@@
+
+if (...)
+  \(memset@ok\|memzero_explicit@ok\)(...);
+
+@r depends on !patch forall@
+expression E;
+position p : script:python() { relevant(p) };
+position m != cond.ok;
+type T;
+@@
+
+(
+* memset@m((T)E, 0, ...);
+|
+* memzero_explicit@m((T)E, ...);
+)
+  ... when != E
+  when strict
+* \(kfree\|vfree\|kvfree\)(E)@p;
+
+@rp_memzero depends on patch@
+expression E, size;
+position p : script:python() { relevant(p) };
+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);
+|
+- \(vfree\|kvfree\)(E)@p;
++ kvfree_sensitive(E, size);
+)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0],
+  "WARNING: opportunity for kzfree/kvfree_sensitive")
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0],
+  "WARNING: opportunity for kzfree/kvfree_sensitive")
-- 
2.26.2

[PATCH v4] coccinelle: misc: add array_size_dup script to detect missed overflow checks

[Denis Efremov]

Detect an opencoded expression that is used before or after
array_size()/array3_size()/struct_size() to compute the same size.

Signed-off-by: Denis Efremov 
---
Changes in v2:
 - python rules moved next to SmPL patterns
 - assignment operator used
 - struct_size patterns fixed to check only E3, since
   E1, E2 are sizeofs of a structure and a member
   of a structure
Changes in v3:
 - s/overlow/overflow/ typo fixed (thanks, Markus)
 - \(\|\) changed to &\(E1\|E2\)
 - print strings breaks removed
Changes in v4:
 - duplicates warning removed
 - python2 compatability in report& modes added
 - s/down the code/later/ warning changed
 - \(E1\|E2\|subE1\|subE2\) patterns simplified to \(subE1\|subE2\)

 scripts/coccinelle/misc/array_size_dup.cocci | 209 +++
 1 file changed, 209 insertions(+)
 create mode 100644 scripts/coccinelle/misc/array_size_dup.cocci

diff --git a/scripts/coccinelle/misc/array_size_dup.cocci 
b/scripts/coccinelle/misc/array_size_dup.cocci
new file mode 100644
index ..d3d635b2d4fc
--- /dev/null
+++ b/scripts/coccinelle/misc/array_size_dup.cocci
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for array_size(), array3_size(), struct_size() duplicates.
+/// Three types of patterns for these functions:
+///  1. An opencoded expression is used before array_size() to compute the 
same size
+///  2. An opencoded expression is used after array_size() to compute the same 
size
+/// From security point of view only first case is relevant. These functions
+/// perform arithmetic overflow check. Thus, if we use an opencoded expression
+/// before a call to the *_size() function we can miss an overflow.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers --no-loops
+
+virtual context
+virtual report
+virtual org
+
+@as@
+expression E1, E2;
+@@
+
+array_size(E1, E2)
+
+@as_next@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2@p1
+  ... when != \(subE1\|subE2\) aop E3
+  when != &\(subE1\|subE2\)
+* array_size(E1, E2)@p2
+
+@script:python depends on report@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+msg = "WARNING: array_size is used later (line %s) to compute the same size" % 
(p2[0].line)
+coccilib.report.print_report(p1[0], msg)
+
+@script:python depends on org@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+msg = "WARNING: array_size is used later (line %s) to compute the same size" % 
(p2[0].line)
+coccilib.org.print_todo(p1[0], msg)
+
+@as_prev@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(subE1\|subE2\) aop E3
+  when != &\(subE1\|subE2\)
+* E1 * E2@p2
+
+@script:python depends on report@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+msg = "WARNING: array_size is already used (line %s) to compute the same size" 
% (p1[0].line)
+coccilib.report.print_report(p2[0], msg)
+
+@script:python depends on org@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+msg = "WARNING: array_size is already used (line %s) to compute the same size" 
% (p1[0].line)
+coccilib.org.print_todo(p2[0], msg)
+
+@as3@
+expression E1, E2, E3;
+@@
+
+array3_size(E1, E2, E3)
+
+@as3_next@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2 * E3@p1
+  ... when != \(subE1\|subE2\|subE3\) aop E4
+  when != &\(subE1\|subE2\|subE3\)
+* array3_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+msg = "WARNING: array3_size is used later (line %s) to compute the same size" 
% (p2[0].line)
+coccilib.report.print_report(p1[0], msg)
+
+@script:python depends on org@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+msg = "WARNING: array3_size is used later (line %s) to compute the same size" 
% (p2[0].line)
+coccilib.org.print_todo(p1[0], msg)
+
+@as3_prev@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array3_size(E1, E2, E3)@p1
+  ... when != \(subE1\|subE2\|subE3\) aop E4
+  when != &\(subE1\|subE2\|subE3\)
+* E1 * E2 * E3@p2
+
+@script:python depends on report@
+p1 << as3_prev.p1;
+p2 << as3_prev.p2;
+@@
+
+msg = "WARNING: array3_size is already used (line %s) to compute the same 
size" % (p1[0].line)
+coccilib.report.print_report(p2[0], msg)
+
+@script:python depends on org@
+p1 << as3_prev.p1;
+p2 << as3_prev.p2;
+@@
+
+msg = "WARNING: array3_

[PATCH] drm/gma500: Fix direction check in psb_accel_2d_copy()

[Denis Efremov]

psb_accel_2d_copy() checks direction PSB_2D_COPYORDER_BR2TL twice.
Based on psb_accel_2d_copy_direction() results, PSB_2D_COPYORDER_TL2BR
should be checked instead in the second direction check.

Fixes: 4d8d096e9ae8 ("gma500: introduce the framebuffer support code")
Cc: sta...@vger.kernel.org
Signed-off-by: Denis Efremov 
---
 drivers/gpu/drm/gma500/accel_2d.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/accel_2d.c 
b/drivers/gpu/drm/gma500/accel_2d.c
index adc0507545bf..8dc86aac54d2 100644
--- a/drivers/gpu/drm/gma500/accel_2d.c
+++ b/drivers/gpu/drm/gma500/accel_2d.c
@@ -179,8 +179,8 @@ static int psb_accel_2d_copy(struct drm_psb_private 
*dev_priv,
src_x += size_x - 1;
dst_x += size_x - 1;
}
-   if (direction == PSB_2D_COPYORDER_BR2TL ||
-   direction == PSB_2D_COPYORDER_BL2TR) {
+   if (direction == PSB_2D_COPYORDER_BL2TR ||
+   direction == PSB_2D_COPYORDER_TL2BR) {
src_y += size_y - 1;
dst_y += size_y - 1;
}
-- 
2.26.2

[PATCH] drm/radeon: fix fb_div check in ni_init_smc_spll_table()

[Denis Efremov]

clk_s is checked twice in a row in ni_init_smc_spll_table().
fb_div should be checked instead.

Fixes: 69e0b57a91ad ("drm/radeon/kms: add dpm support for cayman (v5)")
Cc: sta...@vger.kernel.org
Signed-off-by: Denis Efremov 
---
 drivers/gpu/drm/radeon/ni_dpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
index b57c37ddd164..c7fbb7932f37 100644
--- a/drivers/gpu/drm/radeon/ni_dpm.c
+++ b/drivers/gpu/drm/radeon/ni_dpm.c
@@ -2127,7 +2127,7 @@ static int ni_init_smc_spll_table(struct radeon_device 
*rdev)
if (clk_s & ~(SMC_NISLANDS_SPLL_DIV_TABLE_CLKS_MASK >> 
SMC_NISLANDS_SPLL_DIV_TABLE_CLKS_SHIFT))
ret = -EINVAL;
 
-   if (clk_s & ~(SMC_NISLANDS_SPLL_DIV_TABLE_CLKS_MASK >> 
SMC_NISLANDS_SPLL_DIV_TABLE_CLKS_SHIFT))
+   if (fb_div & ~(SMC_NISLANDS_SPLL_DIV_TABLE_FBDIV_MASK >> 
SMC_NISLANDS_SPLL_DIV_TABLE_FBDIV_SHIFT))
ret = -EINVAL;
 
if (clk_v & ~(SMC_NISLANDS_SPLL_DIV_TABLE_CLKV_MASK >> 
SMC_NISLANDS_SPLL_DIV_TABLE_CLKV_SHIFT))
-- 
2.26.2

[PATCH] btrfs: tests: remove if duplicate in __check_free_space_extents()

[Denis Efremov]

num_extents is already checked in the next if condition and can
be safely removed.

Signed-off-by: Denis Efremov 
---
 fs/btrfs/tests/free-space-tree-tests.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/tests/free-space-tree-tests.c 
b/fs/btrfs/tests/free-space-tree-tests.c
index 914eea5ba6a7..2c783d2f5228 100644
--- a/fs/btrfs/tests/free-space-tree-tests.c
+++ b/fs/btrfs/tests/free-space-tree-tests.c
@@ -60,8 +60,6 @@ static int __check_free_space_extents(struct 
btrfs_trans_handle *trans,
if (prev_bit == 0 && bit == 1) {
extent_start = offset;
} else if (prev_bit == 1 && bit == 0) {
-   if (i >= num_extents)
-   goto invalid;
if (i >= num_extents ||
extent_start != extents[i].start ||
offset - extent_start != 
extents[i].length)
-- 
2.26.2

Re: [Cocci] [PATCH v3] coccinelle: misc: add array_size_dup script to detect missed overflow checks

[Denis Efremov]

What do you think about removing duplicates warning from the rule?

I mean this kind of warnings: "WARNING: same array_size (line {p1[0].line})"

As for now, I think it's better to not disturb developers with this kind
of things.

Thanks,
Denis

>> +@as_dup@
>> +expression subE1 <= as.E1;
>> +expression subE2 <= as.E2;
>> +expression as.E1, as.E2, E3;
>> +assignment operator aop;
>> +position p1, p2;
>> +@@
>> +
>> +* array_size(E1, E2)@p1
>> +  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
>> +  when != &\(E1\|E2\|subE1\|subE2\)
>> +* array_size(E1, E2)@p2
>> +
>> +@script:python depends on report@
>> +p1 << as_dup.p1;
>> +p2 << as_dup.p2;
>> +@@
>> +
>> +coccilib.report.print_report(p2[0],
>> +f"WARNING: same array_size (line {p1[0].line})")
>> +
>> +@script:python depends on org@
>> +p1 << as_dup.p1;
>> +p2 << as_dup.p2;
>> +@@
>> +
>> +coccilib.org.print_todo(p2[0],
>> +f"WARNING: same array_size (line {p1[0].line})")
>> +

Re: [Cocci] [PATCH v3] coccinelle: misc: add array_size_dup script to detect missed overflow checks

[Denis Efremov]

On 6/21/20 11:56 PM, Julia Lawall wrote:
> Is it a python 2 vs python 3 thing?

Yes, python2 is no longer supported and I
thought it would be safe to use this syntax.

Ok, I will make it portable in v4.

Denis

[PATCH] driver core: Use scnprintf() instead of snprintf() in device show functions

[Denis Efremov]

>From Documentation/filesystems/sysfs.txt:
- 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().

The reason is that snprintf() returns the length the resulting string
would be, assuming the data will fit into the destination array.
scnprintf() returns the length of the string actually created in buf.

device_show_{ulong,int,bool}() functions are reference implementation
and should respect the kernel documentation.

Signed-off-by: Denis Efremov 
---

Actually, I don't know why snprintf() is used here initially. It looks
safe to use sprintf().

 drivers/base/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 67d39a90b45c..2815deb511fc 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1472,7 +1472,7 @@ ssize_t device_show_ulong(struct device *dev,
  char *buf)
 {
struct dev_ext_attribute *ea = to_ext_attr(attr);
-   return snprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
+   return scnprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
 }
 EXPORT_SYMBOL_GPL(device_show_ulong);
 
@@ -1502,7 +1502,7 @@ ssize_t device_show_int(struct device *dev,
 {
struct dev_ext_attribute *ea = to_ext_attr(attr);
 
-   return snprintf(buf, PAGE_SIZE, "%d\n", *(int *)(ea->var));
+   return scnprintf(buf, PAGE_SIZE, "%d\n", *(int *)(ea->var));
 }
 EXPORT_SYMBOL_GPL(device_show_int);
 
@@ -1523,7 +1523,7 @@ ssize_t device_show_bool(struct device *dev, struct 
device_attribute *attr,
 {
struct dev_ext_attribute *ea = to_ext_attr(attr);
 
-   return snprintf(buf, PAGE_SIZE, "%d\n", *(bool *)(ea->var));
+   return scnprintf(buf, PAGE_SIZE, "%d\n", *(bool *)(ea->var));
 }
 EXPORT_SYMBOL_GPL(device_show_bool);
 
-- 
2.26.2

Re: ERROR: modpost: "__mulsi3" undefined!

[Denis Efremov]

On 6/19/20 7:17 PM, kernel test robot wrote:
> Hi Denis,
> 
> First bad commit (maybe != root cause):
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
> master
> head:   5e857ce6eae7ca21b2055cca4885545e29228fe2
> commit: e4a42c82e943b97ce124539fcd7a47445b43fa0d kbuild: fix broken builds 
> because of GZIP,BZIP2,LZOP variables
> date:   8 days ago
> config: openrisc-randconfig-c022-20200619 (attached as .config)
> compiler: or1k-linux-gcc (GCC) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot 
> 
> All errors (new ones prefixed by >>, old ones prefixed by <<):
> 

> ERROR: modpost: "__mulsi3" [drivers/power/supply/pcf50633-charger.ko] 
> undefined!
> ERROR: modpost: "__mulsi3" [drivers/power/supply/max17042_battery.ko] 
> undefined!
>>> ERROR: modpost: "__mulsi3" [drivers/power/supply/max17040_battery.ko] 
>>> undefined!

I'm not sure, but this report looks to me like the error was already in code 
before
and the patch only rearranges the report.

> ERROR: modpost: "__mulsi3" [drivers/power/supply/da9150-fg.ko] undefined!
> ERROR: modpost: "__mulsi3" [drivers/power/supply/bq27xxx_battery.ko] 
> undefined!


I found a similar reports:
https://lkml.org/lkml/2020/6/19/341
https://lkml.org/lkml/2019/12/11/2195
https://lkml.org/lkml/2019/12/11/1977

Thanks,
Denis

[PATCH] coccinelle: api/kstrdup: fix coccinelle position

[Denis Efremov]

There is a typo in rule r2. Position p1 should be attached to kzalloc()
call.

Fixes: 29a36d4dec6c ("scripts/coccinelle: improve the coverage of some semantic 
patches")
Signed-off-by: Denis Efremov 
---
 scripts/coccinelle/api/kstrdup.cocci | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/coccinelle/api/kstrdup.cocci 
b/scripts/coccinelle/api/kstrdup.cocci
index 19f2645e6076..3c6dc5469ee4 100644
--- a/scripts/coccinelle/api/kstrdup.cocci
+++ b/scripts/coccinelle/api/kstrdup.cocci
@@ -66,7 +66,7 @@ position p1,p2;
 
 *   x = strlen(from) + 1;
 ... when != \( x = E1 \| from = E1 \)
-*   to = \(kmalloc@p1\|kzalloc@p2\)(x,flag);
+*   to = \(kmalloc@p1\|kzalloc@p1\)(x,flag);
 ... when != \(x = E2 \| from = E2 \| to = E2 \)
 if (to==NULL || ...) S
 ... when != \(x = E3 \| from = E3 \| to = E3 \)
-- 
2.26.2

[PATCH v3] coccinelle: misc: add array_size_dup script to detect missed overflow checks

[Denis Efremov]

Detect an opencoded expression that is used before or after
array_size()/array3_size()/struct_size() to compute the same size.

Cc: Gustavo A. R. Silva 
Cc: Kees Cook 
Signed-off-by: Denis Efremov 
---
Changes in v2:
 - python rules moved next to SmPL patterns
 - assignment operator used
 - struct_size patterns fixed to check only E3, since
   E1, E2 are sizeofs of a structure and a member
   of a structure
Changes in v3:
 - s/overlow/overflow/ typo fixed (thanks, Markus)
 - \(\|\) changed to &\(E1\|E2\)
 - print strings breaks removed

 scripts/coccinelle/misc/array_size_dup.cocci | 297 +++
 1 file changed, 297 insertions(+)
 create mode 100644 scripts/coccinelle/misc/array_size_dup.cocci

diff --git a/scripts/coccinelle/misc/array_size_dup.cocci 
b/scripts/coccinelle/misc/array_size_dup.cocci
new file mode 100644
index ..d03740257e97
--- /dev/null
+++ b/scripts/coccinelle/misc/array_size_dup.cocci
@@ -0,0 +1,297 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for array_size(), array3_size(), struct_size() duplicates.
+/// Three types of patterns for these functions:
+///  1. An opencoded expression is used before array_size() to compute the 
same size
+///  2. An opencoded expression is used after array_size() to compute the same 
size
+///  3. Consecutive calls of array_size() with the same values
+/// From security point of view only first case is relevant. These functions
+/// perform arithmetic overflow check. Thus, if we use an opencoded expression
+/// before a call to the *_size() function we can miss an overflow.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers --no-loops
+
+virtual context
+virtual report
+virtual org
+
+@as@
+expression E1, E2;
+@@
+
+array_size(E1, E2)
+
+@as_next@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2@p1
+  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
+  when != &\(E1\|E2\|subE1\|subE2\)
+* array_size(E1, E2)@p2
+
+@script:python depends on report@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+coccilib.report.print_report(p1[0],
+f"WARNING: array_size is used down the code (line {p2[0].line}) to compute the 
same size")
+
+@script:python depends on org@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+coccilib.org.print_todo(p1[0],
+f"WARNING: array_size is used down the code (line {p2[0].line}) to compute the 
same size")
+
+@as_prev@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
+  when != &\(E1\|E2\|subE1\|subE2\)
+* E1 * E2@p2
+
+@script:python depends on report@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: array_size is already used (line {p1[0].line}) to compute the same 
size")
+
+@script:python depends on org@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: array_size is already used (line {p1[0].line}) to compute the same 
size")
+
+@as_dup@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
+  when != &\(E1\|E2\|subE1\|subE2\)
+* array_size(E1, E2)@p2
+
+@script:python depends on report@
+p1 << as_dup.p1;
+p2 << as_dup.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: same array_size (line {p1[0].line})")
+
+@script:python depends on org@
+p1 << as_dup.p1;
+p2 << as_dup.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: same array_size (line {p1[0].line})")
+
+@as3@
+expression E1, E2, E3;
+@@
+
+array3_size(E1, E2, E3)
+
+@as3_next@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2 * E3@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
+  when != &\(E1\|E2\|E3\|subE1\|subE2\|subE3\)
+* array3_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+coccilib.report.print_report(p1[0],
+f"WARNING: array3_size is used down the code (line {p2[0].line}) to compute 
the same size")
+
+@script:python depends on org@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+coccilib.org.print_todo(p1[0],
+f"WARNING: array3_size is used down the code (line {p2[0].line}) to compute 
the same size")
+
+@as3_prev@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3

Re: [PATCH v2] coccinelle: misc: add array_size_dup script to detect missed overflow checks

[Denis Efremov]

Hi,

On 6/18/20 2:34 PM, Markus Elfring wrote:
> Why did you repeat a typo from the previous patch subject?

Where is the typo? I can't handle your suggestions because your mails constantly
break the threads. I just can't find them after due to missed/wrong In-Reply-To
headers. Again, this mail doesn't contain In-Reply-To header and highly likely I
will miss it when I will prepare next version of the patch.


>> +expression subE1 <= as.E1;
>> +expression subE2 <= as.E2;
>> +expression as.E1, as.E2, E3;
> 
> How do you think about to use the following SmPL code variant?
> 
> expression subE1 <= as.E1, subE2 <= as.E2, as.E1, as.E2, E3;

It's less readable and harder to review.

> 
>> +  when != \(\|\|\|\)
> 
> I suggest to move the ampersand before the disjunction in such
> SmPL code exclusion specifications.
> 
> +  when != & \(E1 \| E2 \| subE1 \| subE2\)

Ok, I will fix this if there will be next version.

> 
> 
>> +coccilib.report.print_report(p2[0],
>> +f"WARNING: array_size is already used (line {p1[0].line}) to compute \
>> +the same size")
> 
> I would prefer an other code formatting at such places.
> 
> +coccilib.report.print_report(p2[0],
> + f"WARNING: array_size is already used (line 
> {p1[0].line}) to compute the same size.")
> 

No. It's pointless to break the line to save 5 chars this way.

I can use instead:

coccilib.report.print_report(p2[0], f"WARNING: array_size is already used (line 
{p1[0].line}) to compute the same size")

or

msg = f"WARNING: array_size is already used (line {p1[0].line}) to compute the 
same size"
coccilib.report.print_report(p2[0], msg)

or

coccilib.report.print_report(p2[0],
f"WARNING: array_size is already used (line {p1[0].line}) to compute the same 
size")

And I prefer the last one if Julia will allow me to use more than 80 chars in 
print string.

Thanks,
Denis

[PATCH v2] coccinelle: misc: add array_size_dup script to detect missed overlow checks

[Denis Efremov]

Detect an opencoded expression that is used before or after
array_size()/array3_size()/struct_size() to compute the same size.

Cc: Gustavo A. R. Silva 
Cc: Kees Cook 
Signed-off-by: Denis Efremov 
---
Changes in v2:
 - python rules moved next to SmPL patterns
 - assignment operator used
 - struct_size patterns fixed to check only E3, since
   E1, E2 are sizeofs of a structure and a member
   of a structure

 scripts/coccinelle/misc/array_size_dup.cocci | 309 +++
 1 file changed, 309 insertions(+)
 create mode 100644 scripts/coccinelle/misc/array_size_dup.cocci

diff --git a/scripts/coccinelle/misc/array_size_dup.cocci 
b/scripts/coccinelle/misc/array_size_dup.cocci
new file mode 100644
index ..c5214310285c
--- /dev/null
+++ b/scripts/coccinelle/misc/array_size_dup.cocci
@@ -0,0 +1,309 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for array_size(), array3_size(), struct_size() duplicates.
+/// Three types of patterns for these functions:
+///  1. An opencoded expression is used before array_size() to compute the 
same size
+///  2. An opencoded expression is used after array_size() to compute the same 
size
+///  3. Consecutive calls of array_size() with the same values
+/// From security point of view only first case is relevant. These functions
+/// perform arithmetic overflow check. Thus, if we use an opencoded expression
+/// before a call to the *_size() function we can miss an overflow.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers --no-loops
+
+virtual context
+virtual report
+virtual org
+
+@as@
+expression E1, E2;
+@@
+
+array_size(E1, E2)
+
+@as_next@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2@p1
+  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
+  when != \(\|\|\|\)
+* array_size(E1, E2)@p2
+
+@script:python depends on report@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+coccilib.report.print_report(p1[0],
+f"WARNING: array_size is used down the code (line {p2[0].line}) to compute \
+the same size")
+
+@script:python depends on org@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+coccilib.org.print_todo(p1[0],
+f"WARNING: array_size is used down the code (line {p2[0].line}) to compute \
+the same size")
+
+@as_prev@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
+  when != \(\|\|\|\)
+* E1 * E2@p2
+
+@script:python depends on report@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: array_size is already used (line {p1[0].line}) to compute \
+the same size")
+
+@script:python depends on org@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: array_size is already used (line {p1[0].line}) to compute \
+the same size")
+
+@as_dup@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
+  when != \(\|\|\|\)
+* array_size(E1, E2)@p2
+
+@script:python depends on report@
+p1 << as_dup.p1;
+p2 << as_dup.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: same array_size (line {p1[0].line})")
+
+@script:python depends on org@
+p1 << as_dup.p1;
+p2 << as_dup.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: same array_size (line {p1[0].line})")
+
+@as3@
+expression E1, E2, E3;
+@@
+
+array3_size(E1, E2, E3)
+
+@as3_next@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2 * E3@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
+  when != \(\|\|\|\|\|\)
+* array3_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+coccilib.report.print_report(p1[0],
+f"WARNING: array3_size is used down the code (line {p2[0].line}) to compute \
+the same size")
+
+@script:python depends on org@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+coccilib.org.print_todo(p1[0],
+f"WARNING: array3_size is used down the code (line {p2[0].line}) to compute \
+the same size")
+
+@as3_prev@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array3_size(E1, E2, E3)@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
+  when != \(\|\|\|\|\|\)
+* E1 * E2 * E

Re: [Cocci] [PATCH v3] coccinelle: api: add kzfree script

[Denis Efremov]

>> +@rp_memset depends on patch@
>> +expression E, size;
>> +position p : script:python() { relevant(p) };
>> +type T;
>> +@@
>> +
>> +- memset((T)E, size)@p;
> 
> This is missing a 0 argument.
> 

Thanks, I will send v4.

> 
> 
>> +  ... when != E
>> +  when strict
>> +(
>> +- kfree(E);
>> ++ kzfree(E);
>> +|
>> +- \(vfree\|kvfree\)(E);
>> ++ kvfree_sensitive(E, size);
>> +)
> 
> I'm not sure why you want kzfree in the first case, but kvfree_sensitive
> in the second case.
> 

As for now in kernel:

memset(E,0,...) && kfree(E) is kzfree()

There are no vzfree or kvzfree functions.
Thus, we use kvfree_sensitive().

Maybe it's worth to wait for this patchset:
https://lkml.org/lkml/2020/6/16/1163

With it the rule will use:

(
- kfree(E);
+ kfree_sensitive(E);
|
- \(vfree\|kvfree\)(E);
+ kvfree_sensitive(E, size);
)

Thanks,
Denis

Re: [PATCH v4 0/3] mm, treewide: Rename kzfree() to kfree_sensitive()

[Denis Efremov]

On 6/16/20 9:53 PM, Joe Perches wrote:
> On Mon, 2020-06-15 at 21:57 -0400, Waiman Long wrote:
>>  v4:
>>   - Break out the memzero_explicit() change as suggested by Dan Carpenter
>> so that it can be backported to stable.
>>   - Drop the "crypto: Remove unnecessary memzero_explicit()" patch for
>> now as there can be a bit more discussion on what is best. It will be
>> introduced as a separate patch later on after this one is merged.
> 
> To this larger audience and last week without reply:
> https://lore.kernel.org/lkml/573b3fbd5927c643920e1364230c296b23e7584d.ca...@perches.com/
> 
> Are there _any_ fastpath uses of kfree or vfree?
> 
> Many patches have been posted recently to fix mispairings
> of specific types of alloc and free functions.

I've prepared a coccinelle script to highlight these mispairings in a function
a couple of days ago: https://lkml.org/lkml/2020/6/5/953
I've listed all the fixes in the commit message. 

Not so many mispairings actually, and most of them are harmless like:
kmalloc(E) -> kvfree(E)

However, coccinelle script can't detect cross-functions mispairings, i.e.
allocation in one function, free in another funtion.

Thanks,
Denis

Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks

[Denis Efremov]

On 6/17/20 11:30 PM, Julia Lawall wrote:
> 
> 
> On Mon, 15 Jun 2020, Denis Efremov wrote:
> 
>> Detect an opencoded expression that is used before or after
>> array_size()/array3_size()/struct_size() to compute the same size.
> 
> This would benefit from the assignemnt operator metavariables as well.
> 
> Also, it could be better to put the python rules up next the SmPL pattern
> matching rules that they are associated with.
> 

Thanks, I will send v2.
Here is the KSPP ticket with patches https://github.com/KSPP/linux/issues/83 

Re: [Cocci] [PATCH] coccinelle: api: add device_attr_show script

[Denis Efremov]

On 6/17/20 11:27 PM, Julia Lawall wrote:
> 
> 
> On Mon, 15 Jun 2020, Denis Efremov wrote:
> 
>> According to the documentation[1] show() methods of device attributes
>> should return the number of bytes printed into the buffer. This is
>> the return value of scnprintf(). show() must not use snprintf()
>> when formatting the value to be returned to user space. snprintf()
>> returns the length the resulting string would be, assuming it all
>> fit into the destination array[2]. scnprintf() return the length of
>> the string actually created in buf. If one can guarantee that an
>> overflow will never happen sprintf() can be used otherwise scnprintf().
> 
> The semantic patch looks fine.  Do you have any accepted patches from
> this?

It's not my patches, but:

3f9f8daad342 cpuidle: sysfs: Fix the overlap for showing available governors
117e2cb3 sparc: use scnprintf() in show_pciobppath_attr() in vio.c
03a1b56f501e sparc: use scnprintf() in show_pciobppath_attr() in pci.c
3dee04262898 iio: tsl2772: Use scnprintf() for avoiding potential buffer 
overflow
dbdd24eaac4e edd: Use scnprintf() for avoiding potential buffer overflow
abdd9feb45ed btrfs: sysfs: Use scnprintf() instead of snprintf()
f21431f2de33 thermal: int340x_thermal: Use scnprintf() for avoiding potential 
buffer overflow
40501c70e3f0 s390/zcrypt: replace snprintf/sprintf with scnprintf
eb3e064b8dd1 s390/zcrypt: Use scnprintf() for avoiding potential buffer overflow
06b522d6de9d video: uvesafb: Use scnprintf() for avoiding potential buffer 
overflow
bf1b615ad97e video: omapfb: Use scnprintf() for avoiding potential buffer 
overflow
b40e288bfb53 platform/x86: sony-laptop: Use scnprintf() for avoiding potential 
buffer overflow
ef21e1750158 ALSA: Use scnprintf() instead of snprintf() for show

and many more

Thanks,
Denis

Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks

[Denis Efremov]

> 
> Awesome! I'll take a look into this. :)
> 
Here is another script for your #83 ticket.
Currently, it issues 598 warnings.

// SPDX-License-Identifier: GPL-2.0-only
///
/// Check for missing overflow checks in allocation functions.
/// Low confidence because it's pointless to check for overflow
/// relatively small allocations.
///
// Confidence: Low
// Copyright: (C) 2020 Denis Efremov ISPRAS
// Options: --no-includes --include-headers

virtual patch
virtual context
virtual org
virtual report

@depends on patch@
expression E1, E2, E3, E4, size;
@@

(
- size = E1 * E2;
+ size = array_size(E1, E2);
|
- size = E1 * E2 * E3;
+ size = array3_size(E1, E2, E3);
|
- size = E1 * E2 + E3;
+ size = struct_size(E1, E2, E3);
)
  ... when != size = E4
  when != size += E4
  when != size -= E4
  when != size *= E4
  when != 
  \(kmalloc\|krealloc\|kzalloc\|kzalloc_node\|
vmalloc\|vzalloc\|vzalloc_node\|
kvmalloc\|kvzalloc\|kvzalloc_node\|
sock_kmalloc\|
f2fs_kmalloc\|f2fs_kzalloc\|f2fs_kvmalloc\|f2fs_kvzalloc\|
devm_kmalloc\|devm_kzalloc\)
  (..., size, ...)

@r depends on !patch@
expression E1, E2, E3, E4, size;
position p;
@@

(
* size = E1 * E2;@p
|
* size = E1 * E2 * E3;@p
|
* size = E1 * E2 + E3;@p
)
  ... when != size = E4
  when != size += E4
  when != size -= E4
  when != size *= E4
  when != 
* \(kmalloc\|krealloc\|kzalloc\|kzalloc_node\|
vmalloc\|vzalloc\|vzalloc_node\|
kvmalloc\|kvzalloc\|kvzalloc_node\|
sock_kmalloc\|
f2fs_kmalloc\|f2fs_kzalloc\|f2fs_kvmalloc\|f2fs_kvzalloc\|
devm_kmalloc\|devm_kzalloc\)
  (..., size, ...)

@script:python depends on report@
p << r.p;
@@

coccilib.report.print_report(p[0], "WARNING: missing overflow check")

@script:python depends on org@
p << r.p;
@@

coccilib.org.print_todo(p[0], "WARNING: missing overflow check")

Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks

[Denis Efremov]

> 
> 
> Awesome! I'll take a look into this. :)
> 

It would be helpful to get a feedback from you after.
What kind of warnings are helpful and what are not?
"duplicate calls" and "opencoded expression after array_size()" look doubtful 
to me.
I think that maintainers will not like these patches.

Thanks,
Denis

Re: [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks

[Denis Efremov]

On 6/15/20 9:23 PM, Kees Cook wrote:
> On Mon, Jun 15, 2020 at 01:20:45PM +0300, Denis Efremov wrote:
>> Detect an opencoded expression that is used before or after
>> array_size()/array3_size()/struct_size() to compute the same size.
>>
>> Cc: Kees Cook 
>> Signed-off-by: Denis Efremov 
> 
> Oh, very cool! How much does this find currently?
> 

opencoded expression before the function call:
./drivers/net/ethernet/cavium/liquidio/request_manager.c:98:34-59: WARNING: 
array_size is used down the code (line 103) to compute the same size
./drivers/media/test-drivers/vivid/vivid-core.c:1120:26-34: WARNING: array_size 
is used down the code (line 1122) to compute the same size
./drivers/scsi/megaraid/megaraid_sas_fusion.c:5184:11-31: WARNING: array_size 
is used down the code (line 5191) to compute the same size
./drivers/scsi/megaraid/megaraid_sas_fusion.c:5200:2-37: WARNING: array_size is 
used down the code (line 5207) to compute the same size
./fs/cifs/misc.c:853:17-39: WARNING: array_size is used down the code (line 
858) to compute the same size
./fs/cifs/misc.c:863:17-38: WARNING: array_size is used down the code (line 
868) to compute the same size
./drivers/scsi/fnic/fnic_trace.c:562:25-48: WARNING: array_size is used down 
the code (line 566) to compute the same size

opencoded expression after the function call:
./net/ethtool/ioctl.c:1976:55-66: WARNING: array_size is already used (line 
1957) to compute the same size
./net/ethtool/ioctl.c:1921:55-66: WARNING: array_size is already used (line 
1909) to compute the same size
./drivers/net/ethernet/cavium/liquidio/request_manager.c:111:29-54: WARNING: 
array_size is already used (line 103) to compute the same size
./drivers/staging/rts5208/ms.c:2309:55-56: WARNING: array_size is already used 
(line 2305) to compute the same size
./drivers/video/fbdev/core/fbcon.c:642:52-53: WARNING: array3_size is already 
used (line 638) to compute the same size
./drivers/video/fbdev/core/fbcon.c:679:47-48: WARNING: array3_size is already 
used (line 638) to compute the same size
./drivers/usb/misc/sisusbvga/sisusb_con.c:1229:54-56: WARNING: array_size is 
already used (line 1226) to compute the same size
./fs/afs/cmservice.c:271:45-46: WARNING: array3_size is already used (line 267) 
to compute the same size
./drivers/mtd/ftl.c:270:49-65: WARNING: array_size is already used (line 266) 
to compute the same size
./drivers/scsi/qla2xxx/tcm_qla2xxx.c:1608:6-42: WARNING: array_size is already 
used (line 1605) to compute the same size
./drivers/scsi/qla2xxx/tcm_qla2xxx.c:1613:8-44: WARNING: array_size is already 
used (line 1605) to compute the same size
./drivers/net/ppp/bsd_comp.c:439:13-37: WARNING: array_size is already used 
(line 409) to compute the same size
./drivers/net/wireless/ath/ath5k/debug.c:957:20-21: WARNING: array_size is 
already used (line 934) to compute the same size
./drivers/scsi/fnic/fnic_trace.c:575:3-26: WARNING: array_size is already used 
(line 566) to compute the same size
./drivers/scsi/fnic/fnic_trace.c:592:32-53: WARNING: array_size is already used 
(line 580) to compute the same size
./drivers/scsi/fnic/fnic_trace.c:504:30-51: WARNING: array_size is already used 
(line 492) to compute the same size
./drivers/staging/rts5208/rtsx_chip.c:1475:17-18: WARNING: array_size is 
already used (line 1458) to compute the same size
./kernel/kexec_file.c:917:8-25: WARNING: array_size is already used (line 913) 
to compute the same size
./drivers/rapidio/devices/rio_mport_cdev.c:984:8-25: WARNING: array_size is 
already used (line 978) to compute the same size
./fs/reiserfs/bitmap.c:1463:22-37: WARNING: array_size is already used (line 
1459) to compute the same size

duplicate calls:
./drivers/media/test-drivers/vivid/vivid-core.c:1125:59-60: WARNING: same 
array_size (line 1122)
./drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:142:36-37: WARNING: same 
array_size (line 138)
./drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:129:41-42: WARNING: same 
array3_size (line 123)
./drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:146:40-41: WARNING: same 
array3_size (line 123)
./drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:146:40-41: WARNING: same 
array3_size (line 129)
./drivers/net/ethernet/cavium/liquidio/octeon_droq.c:289:27-28: WARNING: same 
array_size (line 284)
./drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c:857:59-60: WARNING: same 
struct_size (line 854)
./fs/f2fs/super.c:3478:34-35: WARNING: same array_size (line 3478)
./drivers/net/wireless/zydas/zd1211rw/zd_usb.c:1637:45-46: WARNING: same 
struct_size (line 1634)
./drivers/net/ethernet/netronome/nfp/flower/cmsg.c:221:49-50: WARNING: same 
struct_size (line 219)
./drivers/staging/rts5208/rtsx_chip.c:1458:36-37: WARNING: same array_size 
(line 1454)
./drivers/net/ethernet/neterion/vxge/vxge-config.c:2664:59-60: WARNING: same 
array_size (line 2654)

Re: [PATCH] coccinelle: api: add kzfree script

[Denis Efremov]

On 6/15/20 3:03 PM, Dan Carpenter wrote:
> On Sun, Jun 14, 2020 at 10:42:54PM +0300, Denis Efremov wrote:
>> On 6/4/20 7:27 PM, Joe Perches wrote:
>>> On Thu, 2020-06-04 at 17:08 +0300, Denis Efremov wrote:
>>>> Check for memset() with 0 followed by kfree().
>>>
>>> Perhaps those uses should be memzero_explicit or kvfree_sensitive.
>>>
>>
>> Is it safe to suggest to use kzfree instead of memzero_explicit && kfree?
>> Or it would be better to use kvfree_sensitive in this case.
>>
>> kzfree uses memset(0) with no barrier_data.
> 
> Yeah.  That seems buggy.  It should have a barrier.  Also I thought I
> saw somewhere that Linus doesn't like the name and so that's why we have
> the _sensitive() name?
> 

Oh, there are already patches for renaming kzfree to kfree_sensitive.

https://lkml.org/lkml/2020/4/13/729

It seems they are not accepted despite multiple acks, through.

Thanks,
Denis

[PATCH] coccinelle: api: add device_attr_show script

[Denis Efremov]

According to the documentation[1] show() methods of device attributes
should return the number of bytes printed into the buffer. This is
the return value of scnprintf(). show() must not use snprintf()
when formatting the value to be returned to user space. snprintf()
returns the length the resulting string would be, assuming it all
fit into the destination array[2]. scnprintf() return the length of
the string actually created in buf. If one can guarantee that an
overflow will never happen sprintf() can be used otherwise scnprintf().

[1] Documentation/filesystems/sysfs.txt
[2] "snprintf() confusion" https://lwn.net/Articles/69419/

Signed-off-by: Denis Efremov 
---
 scripts/coccinelle/api/device_attr_show.cocci | 55 +++
 1 file changed, 55 insertions(+)
 create mode 100644 scripts/coccinelle/api/device_attr_show.cocci

diff --git a/scripts/coccinelle/api/device_attr_show.cocci 
b/scripts/coccinelle/api/device_attr_show.cocci
new file mode 100644
index ..d8ec4bb8ac41
--- /dev/null
+++ b/scripts/coccinelle/api/device_attr_show.cocci
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// From Documentation/filesystems/sysfs.txt:
+///  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().
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual report
+virtual org
+virtual context
+virtual patch
+
+@r depends on !patch@
+identifier show, dev, attr, buf;
+position p;
+@@
+
+ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+   <...
+*  return snprintf@p(...);
+   ...>
+}
+
+@rp depends on patch@
+identifier show, dev, attr, buf;
+@@
+
+ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+   <...
+   return
+-  snprintf
++  scnprintf
+   (...);
+   ...>
+}
+
+@script: python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING: use scnprintf or sprintf")
+
+@script: python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING: use scnprintf or sprintf")
-- 
2.26.2

[PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks

[Denis Efremov]

Detect an opencoded expression that is used before or after
array_size()/array3_size()/struct_size() to compute the same size.

Cc: Kees Cook 
Signed-off-by: Denis Efremov 
---
 scripts/coccinelle/misc/array_size_dup.cocci | 347 +++
 1 file changed, 347 insertions(+)
 create mode 100644 scripts/coccinelle/misc/array_size_dup.cocci

diff --git a/scripts/coccinelle/misc/array_size_dup.cocci 
b/scripts/coccinelle/misc/array_size_dup.cocci
new file mode 100644
index ..08919a938754
--- /dev/null
+++ b/scripts/coccinelle/misc/array_size_dup.cocci
@@ -0,0 +1,347 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for array_size(), array3_size(), struct_size() duplicates.
+/// Three types of patterns for these functions:
+///  1. An opencoded expression is used before array_size() to compute the 
same size
+///  2. An opencoded expression is used after array_size() to compute the same 
size
+///  3. Consecutive calls of array_size() with the same values
+/// From security point of view only first case is relevant. These functions
+/// perform arithmetic overflow check. Thus, if we use an opencoded expression
+/// before a call to the *_size() function we can miss an overflow.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers --no-loops
+
+virtual context
+virtual report
+virtual org
+
+@as@
+expression E1, E2;
+@@
+
+array_size(E1, E2)
+
+@as_next@
+expression subE1 <= as.E1;
+expression as.E1;
+expression subE2 <= as.E2;
+expression as.E2;
+expression E3;
+position p1, p2;
+@@
+
+* E1 * E2@p1
+  ... when != \(E1\|E2\|subE1\|subE2\)=E3
+  when != \(E1\|E2\|subE1\|subE2\)+=E3
+  when != \(E1\|E2\|subE1\|subE2\)-=E3
+  when != \(E1\|E2\|subE1\|subE2\)*=E3
+  when != \(\|\|\|\)
+* array_size(E1, E2)@p2
+
+@as_prev@
+expression subE1 <= as.E1;
+expression as.E1;
+expression subE2 <= as.E2;
+expression as.E2;
+expression E3;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(E1\|E2\|subE1\|subE2\)=E3
+  when != \(E1\|E2\|subE1\|subE2\)+=E3
+  when != \(E1\|E2\|subE1\|subE2\)-=E3
+  when != \(E1\|E2\|subE1\|subE2\)*=E3
+  when != \(\|\|\|\)
+* E1 * E2@p2
+
+@as_dup@
+expression subE1 <= as.E1;
+expression as.E1;
+expression subE2 <= as.E2;
+expression as.E2;
+expression E3;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(E1\|E2\|subE1\|subE2\)=E3
+  when != \(E1\|E2\|subE1\|subE2\)+=E3
+  when != \(E1\|E2\|subE1\|subE2\)-=E3
+  when != \(E1\|E2\|subE1\|subE2\)*=E3
+  when != \(\|\|\|\)
+* array_size(E1, E2)@p2
+
+@as3@
+expression E1, E2, E3;
+@@
+
+array3_size(E1, E2, E3)
+
+@as3_next@
+expression subE1 <= as3.E1;
+expression as3.E1;
+expression subE2 <= as3.E2;
+expression as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E3;
+expression E4;
+position p1, p2;
+@@
+
+* E1 * E2 * E3@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
+  when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
+  when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
+  when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
+  when != \(\|\|\|\|\|\)
+* array3_size(E1, E2, E3)@p2
+
+@as3_prev@
+expression subE1 <= as3.E1;
+expression as3.E1;
+expression subE2 <= as3.E2;
+expression as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E3;
+expression E4;
+position p1, p2;
+@@
+
+* array3_size(E1, E2, E3)@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
+  when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
+  when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
+  when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
+  when != \(\|\|\|\|\|\)
+* E1 * E2 * E3@p2
+
+@as3_dup@
+expression subE1 <= as3.E1;
+expression as3.E1;
+expression subE2 <= as3.E2;
+expression as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E3;
+expression E4;
+position p1, p2;
+@@
+
+* array3_size(E1, E2, E3)@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
+  when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
+  when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
+  when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
+  when != \(\|\|\|\|\|\)
+* array3_size(E1, E2, E3)@p2
+
+@ss@
+expression E1, E2, E3;
+@@
+
+struct_size(E1, E2, E3)
+
+@ss_next@
+expression subE1 <= ss.E1;
+expression ss.E1;
+expression subE2 <= ss.E2;
+expression ss.E2;
+expression subE3 <= ss.E3;
+expression ss.E3;
+expression E4;
+position p1, p2;
+@@
+
+* E1 * E2 + E3@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
+  when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
+  when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
+  when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
+  when != \(\|\|\|\|\|\)
+* struct_size(E1, E2, E3)@p2
+
+@ss_prev@
+expression subE1 <= ss.E1;
+expression ss.E1;
+expression subE2 <= ss.E2;
+expression ss.E2;
+expression subE3 <= ss.E3;
+expression ss.E3;
+expression E4;
+position p1, 

[PATCH v3] coccinelle: api: add kzfree script

[Denis Efremov]

Check for memset()/memzero_explicit() followed by kfree()/vfree()/kvfree().

Signed-off-by: Denis Efremov 
---
Changes in v2:
 - memset_explicit() added
 - kvfree_sensitive() added
 - forall added to r1
 - ... between memset and kfree added
Changes in v3:
 - Explicit filter for definitions instead of !(file in "...") conditions
 - type T added to match casts
 - memzero_explicit() patterns fixed
 - additional rule "cond" added to filter false-positives

 scripts/coccinelle/api/kzfree.cocci | 90 +
 1 file changed, 90 insertions(+)
 create mode 100644 scripts/coccinelle/api/kzfree.cocci

diff --git a/scripts/coccinelle/api/kzfree.cocci 
b/scripts/coccinelle/api/kzfree.cocci
new file mode 100644
index ..4758ca5a781e
--- /dev/null
+++ b/scripts/coccinelle/api/kzfree.cocci
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Use kzfree, kvfree_sensitive rather than memset or
+/// memzero_explicit followed by kfree
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: kzfree, kvfree_sensitive
+//
+
+virtual context
+virtual patch
+virtual org
+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'])
+
+def relevant(p):
+return not (filter & {el.current_element for el in p})
+
+@cond@
+position ok;
+@@
+
+if (...)
+  \(memset@ok\|memzero_explicit@ok\)(...);
+
+@r depends on !patch forall@
+expression E;
+position p : script:python() { relevant(p) };
+position m != cond.ok;
+type T;
+@@
+
+(
+* memset@m((T)E, 0, ...);
+|
+* memzero_explicit@m((T)E, ...);
+)
+  ... when != E
+  when strict
+* \(kfree\|vfree\|kvfree\)(E)@p;
+
+@rp_memzero depends on patch@
+expression E, size;
+position p : script:python() { relevant(p) };
+type T;
+@@
+
+- memzero_explicit((T)E, size)@p;
+  ... when != E
+  when strict
+- \(kfree\|vfree\|kvfree\)(E);
++ kvfree_sensitive(E, size);
+
+@rp_memset depends on patch@
+expression E, size;
+position p : script:python() { relevant(p) };
+type T;
+@@
+
+- memset((T)E, size)@p;
+  ... when != E
+  when strict
+(
+- kfree(E);
++ kzfree(E);
+|
+- \(vfree\|kvfree\)(E);
++ kvfree_sensitive(E, size);
+)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0],
+  "WARNING: opportunity for kzfree/kvfree_sensitive")
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0],
+  "WARNING: opportunity for kzfree/kvfree_sensitive")
-- 
2.26.2

Re: [PATCH] coccinelle: api: add kzfree script

[Denis Efremov]

On 6/4/20 7:27 PM, Joe Perches wrote:
> On Thu, 2020-06-04 at 17:08 +0300, Denis Efremov wrote:
>> Check for memset() with 0 followed by kfree().
> 
> Perhaps those uses should be memzero_explicit or kvfree_sensitive.
> 

Is it safe to suggest to use kzfree instead of memzero_explicit && kfree?
Or it would be better to use kvfree_sensitive in this case.

kzfree uses memset(0) with no barrier_data.

For example:
diff -u -p a/drivers/crypto/inside-secure/safexcel_hash.c 
b/drivers/crypto/inside-secure/safexcel_hash.c
--- a/drivers/crypto/inside-secure/safexcel_hash.c
+++ b/drivers/crypto/inside-secure/safexcel_hash.c
@@ -1081,8 +1081,7 @@ static int safexcel_hmac_init_pad(struct
}
 
/* Avoid leaking */
-   memzero_explicit(keydup, keylen);
-   kfree(keydup);
+   kzfree(keydup);
 
if (ret)
return ret;

Thanks,
Denis

[PATCH v2] coccinelle: api: add kvfree script

[Denis Efremov]

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

 scripts/coccinelle/api/kvfree.cocci | 227 
 1 file changed, 227 insertions(+)
 create mode 100644 scripts/coccinelle/api/kvfree.cocci

diff --git a/scripts/coccinelle/api/kvfree.cocci 
b/scripts/coccinelle/api/kvfree.cocci
new file mode 100644
index ..9455f9866ad8
--- /dev/null
+++ b/scripts/coccinelle/api/kvfree.cocci
@@ -0,0 +1,227 @@
+// 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: Medium
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual patch
+virtual report
+virtual org
+virtual context
+
+@initialize:python@
+@@
+# low-level memory api
+filter = frozenset(['__vmalloc_area_node'])
+
+def relevant(p):
+return not (filter & {el.current_element for el in p})
+
+@choice@
+expression E, E1;
+position kok, vok;
+@@
+
+(
+  if (...) {
+...
+E = \(kmalloc@kok\|kzalloc@kok\|krealloc@kok\|kcalloc@kok\|
+  kmalloc_node@kok\|kzalloc_node@kok\|kmalloc_array@kok\|
+  kmalloc_array_node@kok\|kcalloc_node@kok\)(...)
+...
+  } else {
+...
+E = \(vmalloc@vok\|vzalloc@vok\|vmalloc_user@vok\|vmalloc_node@vok\|
+  vzalloc_node@vok\|vmalloc_exec@vok\|vmalloc_32@vok\|
+  vmalloc_32_user@vok\|__vmalloc@vok\|__vmalloc_node_range@vok\|
+  __vmalloc_node@vok\)(...)
+...
+  }
+|
+  E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(...)
+  ... when != E = E1
+  when any
+  if (\(!E\|E == NULL\)) {
+...
+E = \(vmalloc@vok\|vzalloc@vok\|vmalloc_user@vok\|vmalloc_node@vok\|
+  vzalloc_node@vok\|vmalloc_exec@vok\|vmalloc_32@vok\|
+  vmalloc_32_user@vok\|__vmalloc@vok\|__vmalloc_node_range@vok\|
+  __vmalloc_node@vok\)(...)
+...
+  }
+)
+
+@opportunity depends on !patch@
+expression E, E1, size;
+position p : script:python() { relevant(p) };
+@@
+
+(
+* if (\(size <= E1\|size < E1\|size = E1\|size > E1\) || ...)@p {
+...
+E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+  kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(..., size, ...)
+...
+  } else {
+...
+E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|
+  vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|
+  __vmalloc_node_range\|__vmalloc_node\)(..., size, ...)
+...
+  }
+|
+  E = \(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|
+kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(..., size, ...)
+  ... when != E = E1
+  when != size = E1
+  when any
+* if (\(!E\|E == NULL\))@p {
+...
+E = \(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|
+  vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|
+  __vmalloc_node_range\|__vmalloc_node\)(..., size, ...)
+...
+  }
+)
+
+@vfree depends on !patch@
+expression E;
+position k != choice.kok;
+position p;
+@@
+
+* E = \(kmalloc@k\|kzalloc@k\|krealloc@k\|kcalloc@k\|kmalloc_node@k\|
+kzalloc_node@k\|kmalloc_array@k\|kmalloc_array_node@k\|
+kcalloc_node@k\)(...)
+  ... 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)@p
+
+@pvfree depends on patch exists@
+expression E;
+position k != choice.kok;
+@@
+
+  E = \(kmalloc@k\|kzalloc@k\|krealloc@k\|kcalloc@k\|kmalloc_node@k\|
+kzalloc_node@k\|kmalloc_array@k\|kmalloc_array_node@k\|
+kcalloc_node@k\)(...)
+  ... 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)
++ kfree(E)
+
+@kfree depends on !patch@
+expression E;
+position v != choice.vok;
+position p;
+@@
+
+* E = \(vmalloc@v\|vzalloc@v\|vmalloc_user@v\|vmalloc_node@v\|vzalloc_node@v\|
+vmalloc_exec@v\|vmalloc_32@v\|vmalloc_32_user@v\|__vmalloc@v\|
+__vmalloc_node_range@v\|__vmalloc_node@v\)(...)
+  ... when != !is_vmalloc_addr(E)
+  when any
+* \(kfree\|kzfree\|kvfree\)(E)
+
+@pkfree depends on patch exists@
+expression E;
+position v !=

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Denis Efremov]

On 6/14/20 12:17 PM, Julia Lawall wrote:
> 
> 
> On Sun, 14 Jun 2020, Denis Efremov wrote:
> 
>>
>>
>> On 6/5/20 11:51 PM, Julia Lawall wrote:
>>> Also, there is no need to exceed 80 characters here.  You can put a
>>> newline in the middle of a \( ... \)
>>
>> It's required. Looks like it's impossible to break "when" lines.
> 
> That's true. Sorry for the noise.
> 

Anyway, I will send v2 with other lines fixed.

Thanks,
Denis

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Denis Efremov]

On 6/5/20 11:51 PM, Julia Lawall wrote:
> Also, there is no need to exceed 80 characters here.  You can put a
> newline in the middle of a \( ... \)

It's required. Looks like it's impossible to break "when" lines.

... when != if (...) { ... E = 
\(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\|kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...);
 ... }

Thanks,
Denis

Re: [Cocci] [PATCH v2 4/4] coccinelle: api: add selfcheck for memdup_user rule

[Denis Efremov]

On 6/9/20 7:22 PM, Julia Lawall wrote:
> 
> 
> On Mon, 8 Jun 2020, Denis Efremov wrote:
> 
>> Check that the rule matches vmemdup_user implementation.
>> memdup_user is out of scope because we are not matching
>> kmalloc_track_caller() function.
> 
> Is this a bit over-enginered?

Last patch it's just a PoC. Patches 1-3 are independent from 4.

> More precisely, even if it is nice to check
> that the API definition has the expected behavior, does it make sense to
> do it in one case but not the other?

Yes, I also don't like it. However, I doubt that we need to match
kmalloc_track_caller.

Thanks,
Denis

[PATCH v3] f2fs: use kfree() instead of kvfree() to free superblock data

[Denis Efremov]

Use kfree() instead of kvfree() to free super in read_raw_super_block()
because the memory is allocated with kzalloc() in the function.
Use kfree() instead of kvfree() to free sbi, raw_super in
f2fs_fill_super() and f2fs_put_super() because the memory is allocated
with kzalloc().

Fixes: 5222595d093e ("f2fs: use kvmalloc, if kmalloc is failed")
Signed-off-by: Denis Efremov 
---
Changes in v2:
 - Single patch instead of two separate patches
 - kvfree fixed in f2fs_put_super
Changes in v3:
 - raw_super added to the scope, thanks Chao Yu

 fs/f2fs/super.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 8a9955902d84..7b458268ea09 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1241,7 +1241,7 @@ static void f2fs_put_super(struct super_block *sb)
sb->s_fs_info = NULL;
if (sbi->s_chksum_driver)
crypto_free_shash(sbi->s_chksum_driver);
-   kvfree(sbi->raw_super);
+   kfree(sbi->raw_super);
 
destroy_device_list(sbi);
f2fs_destroy_xattr_caches(sbi);
@@ -1257,7 +1257,7 @@ static void f2fs_put_super(struct super_block *sb)
 #ifdef CONFIG_UNICODE
utf8_unload(sbi->s_encoding);
 #endif
-   kvfree(sbi);
+   kfree(sbi);
 }
 
 int f2fs_sync_fs(struct super_block *sb, int sync)
@@ -3130,7 +3130,7 @@ static int read_raw_super_block(struct f2fs_sb_info *sbi,
 
/* No valid superblock */
if (!*raw_super)
-   kvfree(super);
+   kfree(super);
else
err = 0;
 
@@ -3808,11 +3808,11 @@ static int f2fs_fill_super(struct super_block *sb, void 
*data, int silent)
fscrypt_free_dummy_context(_OPTION(sbi).dummy_enc_ctx);
kvfree(options);
 free_sb_buf:
-   kvfree(raw_super);
+   kfree(raw_super);
 free_sbi:
if (sbi->s_chksum_driver)
crypto_free_shash(sbi->s_chksum_driver);
-   kvfree(sbi);
+   kfree(sbi);
 
/* give only one another chance */
if (retry_cnt > 0 && skip_recovery) {
-- 
2.26.2

Re: [PATCH] kbuild: fix broken builds because of GZIP,BZIP2,LZOP variables

[Denis Efremov]

On 6/8/20 6:36 PM, Adam Borowski wrote:
> On Mon, Jun 08, 2020 at 12:59:44PM +0300, Denis Efremov wrote:
>> Redefine GZIP, BZIP2, LZOP variables as KGZIP, KBZIP2, KLZOP resp.
>> GZIP, BZIP2, LZOP env variables are reserved by the tools. The original
>> attempt to redefine them internally doesn't work in makefiles/scripts
>> intercall scenarios, e.g., "make GZIP=gzip bindeb-pkg" and results in
>> broken builds. There can be other broken build commands because of this,
>> so the universal solution is to use non-reserved env variables for the
>> compression tools.
>>
>> Fixes: 8dfb61dcbace ("kbuild: add variables for compression tools")
> 
> Same said my bisect before I noticed your fix. :)
> 
>> Signed-off-by: Denis Efremov 
> 
> However, I run just basic "make bindeb-pkg" without forcing any variables,
> thus the commit message is wrong.

I would not say it's fully wrong. At least in my case "make bindeb-pkg" builds
successfully (Fedora32/Debian10). I just added to the commit's message the
command "make GZIP=gzip bindeb-pkg" as an example that I think will "reliably"
break the build in any environment.

Thanks,
Denis

[PATCH v2] f2fs: use kfree() instead of kvfree() to free superblock data

[Denis Efremov]

Use kfree() instead of kvfree() to free super in read_raw_super_block()
because the memory is allocated with kzalloc() in the function.
Use kfree() instead of kvfree() to free sbi in f2fs_fill_super() and
f2fs_put_super() because the memory is allocated with kzalloc().

Fixes: 5222595d093e ("f2fs: use kvmalloc, if kmalloc is failed")
Signed-off-by: Denis Efremov 
---
Changes in v2:
 - Single patch instead of two separate patches
 - kvfree fixed in f2fs_put_super

 fs/f2fs/super.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 8a9955902d84..94bfc140bef9 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1257,7 +1257,7 @@ static void f2fs_put_super(struct super_block *sb)
 #ifdef CONFIG_UNICODE
utf8_unload(sbi->s_encoding);
 #endif
-   kvfree(sbi);
+   kfree(sbi);
 }
 
 int f2fs_sync_fs(struct super_block *sb, int sync)
@@ -3130,7 +3130,7 @@ static int read_raw_super_block(struct f2fs_sb_info *sbi,
 
/* No valid superblock */
if (!*raw_super)
-   kvfree(super);
+   kfree(super);
else
err = 0;
 
@@ -3812,7 +3812,7 @@ static int f2fs_fill_super(struct super_block *sb, void 
*data, int silent)
 free_sbi:
if (sbi->s_chksum_driver)
crypto_free_shash(sbi->s_chksum_driver);
-   kvfree(sbi);
+   kfree(sbi);
 
/* give only one another chance */
if (retry_cnt > 0 && skip_recovery) {
-- 
2.26.2

[PATCH v2] drm/panfrost: Use kvfree() to free bo->sgts

[Denis Efremov]

Use kvfree() to free bo->sgts, because the memory is allocated with
kvmalloc_array() in panfrost_mmu_map_fault_addr().

Fixes: 187d2929206e ("drm/panfrost: Add support for GPU heap allocations")
Cc: sta...@vger.kernel.org
Signed-off-by: Denis Efremov 
---
Change in v2:
 - kvfree() fixed in panfrost_gem_free_object(), thanks to Steven Price

 drivers/gpu/drm/panfrost/panfrost_gem.c | 2 +-
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c 
b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 17b654e1eb94..556181ea4a07 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -46,7 +46,7 @@ static void panfrost_gem_free_object(struct drm_gem_object 
*obj)
sg_free_table(>sgts[i]);
}
}
-   kfree(bo->sgts);
+   kvfree(bo->sgts);
}
 
drm_gem_shmem_free_object(obj);
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c 
b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index ed28aeba6d59..3c8ae7411c80 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -486,7 +486,7 @@ static int panfrost_mmu_map_fault_addr(struct 
panfrost_device *pfdev, int as,
pages = kvmalloc_array(bo->base.base.size >> PAGE_SHIFT,
   sizeof(struct page *), GFP_KERNEL | 
__GFP_ZERO);
if (!pages) {
-   kfree(bo->sgts);
+   kvfree(bo->sgts);
bo->sgts = NULL;
mutex_unlock(>base.pages_lock);
ret = -ENOMEM;
-- 
2.26.2

[PATCH v2 2/4] coccinelle: api: extend memdup_user rule with vmemdup_user()

[Denis Efremov]

Add vmemdup_user() transformations to the memdup_user.cocci rule.
Commit 50fd2f298bef ("new primitive: vmemdup_user()") introduced
vmemdup_user(). The function uses kvmalloc with GPF_USER flag.

Signed-off-by: Denis Efremov 
---
 scripts/coccinelle/api/memdup_user.cocci | 45 
 1 file changed, 45 insertions(+)

diff --git a/scripts/coccinelle/api/memdup_user.cocci 
b/scripts/coccinelle/api/memdup_user.cocci
index cadcc2e87881..d15c061a34ab 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -39,6 +39,28 @@ identifier l1,l2;
 -...+>
 -  }
 
+@depends on patch@
+expression from,to,size;
+identifier l1,l2;
+@@
+
+-  to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
++  to = vmemdup_user(from,size);
+   if (
+-  to==NULL
++  IS_ERR(to)
+ || ...) {
+   <+... when != goto l1;
+-  -ENOMEM
++  PTR_ERR(to)
+   ...+>
+   }
+-  if (copy_from_user(to, from, size) != 0) {
+-<+... when != goto l2;
+--EFAULT
+-...+>
+-  }
+
 @r depends on !patch@
 expression from,to,size;
 position p;
@@ -52,6 +74,17 @@ statement S1,S2;
if (copy_from_user(to, from, size) != 0)
S2
 
+@rv depends on !patch@
+expression from,to,size;
+position p;
+statement S1,S2;
+@@
+
+*  to = \(kvmalloc@p\|kvzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
+   if (to==NULL || ...) S1
+   if (copy_from_user(to, from, size) != 0)
+   S2
+
 @script:python depends on org@
 p << r.p;
 @@
@@ -63,3 +96,15 @@ p << r.p;
 @@
 
 coccilib.report.print_report(p[0], "WARNING opportunity for memdup_user")
+
+@script:python depends on org@
+p << rv.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for vmemdup_user")
+
+@script:python depends on report@
+p << rv.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for vmemdup_user")
-- 
2.26.2

[PATCH v2 4/4] coccinelle: api: add selfcheck for memdup_user rule

[Denis Efremov]

Check that the rule matches vmemdup_user implementation.
memdup_user is out of scope because we are not matching
kmalloc_track_caller() function.

Signed-off-by: Denis Efremov 
---
 scripts/coccinelle/api/memdup_user.cocci | 46 ++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/scripts/coccinelle/api/memdup_user.cocci 
b/scripts/coccinelle/api/memdup_user.cocci
index 8621bd98be1e..78fded83b197 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -14,13 +14,24 @@ virtual patch
 virtual context
 virtual org
 virtual report
+virtual selfcheck
 
 @initialize:python@
 @@
-filter = frozenset(['memdup_user', 'vmemdup_user'])
+
+definitions = {
+'memdup_user': 'mm/util.c',
+'vmemdup_user': 'mm/util.c',
+}
+
+filter = frozenset(definitions.keys())
+coccinelle.filtered = set()
+coccinelle.checked_files = set()
 
 def relevant(p):
-return not (filter & {el.current_element for el in p})
+found = filter & {el.current_element for el in p}
+coccinelle.filtered |= found
+return not found
 
 @depends on patch@
 expression from,to,size;
@@ -117,3 +128,34 @@ p << rv.p;
 @@
 
 coccilib.report.print_report(p[0], "WARNING opportunity for vmemdup_user")
+
+@script:python depends on selfcheck@
+@@
+coccinelle.checked_files |= set(definitions.values()) & set(cocci.files())
+
+@finalize:python depends on selfcheck@
+filtered << merge.filtered;
+checked_files << merge.checked_files;
+@@
+
+# Don't check memdup_user because the pattern is not capturing
+# kmalloc_track_caller() calls
+del definitions['memdup_user']
+
+# mapping between checked files and filtered definitions
+found_defns = {}
+for files, funcs in zip(checked_files, filtered):
+   for file in files:
+  found_defns[file] = funcs
+
+# reverse mapping of definitions
+expected_defns = {v : set() for v in definitions.values()}
+for k, v in definitions.items():
+expected_defns[v] |= {k}
+
+for efile, efuncs in expected_defns.items():
+if efile in found_defns:
+not_found = efuncs - found_defns[efile]
+if not_found:
+print('SELF-CHECK: the pattern no longer matches ' \
+ 'definitions {} in file {}'.format(not_found, efile))
-- 
2.26.2

[PATCH v2 3/4] coccinelle: api: filter out memdup_user definitions

[Denis Efremov]

Don't match original implementations.

Signed-off-by: Denis Efremov 
---
 scripts/coccinelle/api/memdup_user.cocci | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/scripts/coccinelle/api/memdup_user.cocci 
b/scripts/coccinelle/api/memdup_user.cocci
index d15c061a34ab..8621bd98be1e 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -15,12 +15,20 @@ virtual context
 virtual org
 virtual report
 
+@initialize:python@
+@@
+filter = frozenset(['memdup_user', 'vmemdup_user'])
+
+def relevant(p):
+return not (filter & {el.current_element for el in p})
+
 @depends on patch@
 expression from,to,size;
 identifier l1,l2;
+position p : script:python() { relevant(p) };
 @@
 
--  to = \(kmalloc\|kzalloc\)
+-  to = \(kmalloc@p\|kzalloc@p\)
(size,\(GFP_KERNEL\|GFP_USER\|
  \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
 +  to = memdup_user(from,size);
@@ -42,9 +50,10 @@ identifier l1,l2;
 @depends on patch@
 expression from,to,size;
 identifier l1,l2;
+position p : script:python() { relevant(p) };
 @@
 
--  to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
+-  to = \(kvmalloc@p\|kvzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
 +  to = vmemdup_user(from,size);
if (
 -  to==NULL
@@ -63,7 +72,7 @@ identifier l1,l2;
 
 @r depends on !patch@
 expression from,to,size;
-position p;
+position p : script:python() { relevant(p) };
 statement S1,S2;
 @@
 
@@ -76,7 +85,7 @@ statement S1,S2;
 
 @rv depends on !patch@
 expression from,to,size;
-position p;
+position p : script:python() { relevant(p) };
 statement S1,S2;
 @@
 
-- 
2.26.2

[PATCH v2 1/4] coccinelle: api: extend memdup_user transformation with GFP_USER

[Denis Efremov]

Match GFP_USER and optional __GFP_NOWARN allocations with
memdup_user.cocci rule.
Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
memdup_user() from GFP_KERNEL to GFP_USER. In almost all cases it
is still a good idea to recommend memdup_user() for GFP_KERNEL
allocations. The motivation behind altering memdup_user() to GFP_USER:
https://lkml.org/lkml/2018/1/6/333

Signed-off-by: Denis Efremov 
---
 scripts/coccinelle/api/memdup_user.cocci | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/scripts/coccinelle/api/memdup_user.cocci 
b/scripts/coccinelle/api/memdup_user.cocci
index c809ab10bbce..cadcc2e87881 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -20,7 +20,9 @@ expression from,to,size;
 identifier l1,l2;
 @@
 
--  to = \(kmalloc\|kzalloc\)(size,GFP_KERNEL);
+-  to = \(kmalloc\|kzalloc\)
+   (size,\(GFP_KERNEL\|GFP_USER\|
+ \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
 +  to = memdup_user(from,size);
if (
 -  to==NULL
@@ -43,7 +45,9 @@ position p;
 statement S1,S2;
 @@
 
-*  to = \(kmalloc@p\|kzalloc@p\)(size,GFP_KERNEL);
+*  to = \(kmalloc@p\|kzalloc@p\)
+   (size,\(GFP_KERNEL\|GFP_USER\|
+ \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));
if (to==NULL || ...) S1
if (copy_from_user(to, from, size) != 0)
S2
-- 
2.26.2

[PATCH v2 0/4] Update memdup_user.cocci

[Denis Efremov]

Add GFP_USER to the allocation flags and handle vmemdup_user().
The third patch supresses memdup_user(), vmemdup_user() functions
detection. Last patch is a proof of concept for the rule selfchecking.
Gives the ability to detect that an open-coded pattern in a function
definition that we search for in the kernel sources changed.

Denis Efremov (4):
  coccinelle: api: extend memdup_user transformation with GFP_USER
  coccinelle: api: extend memdup_user rule with vmemdup_user()
  coccinelle: api: filter out memdup_user definitions
  coccinelle: api: add selfcheck for memdup_user rule

 scripts/coccinelle/api/memdup_user.cocci | 106 ++-
 1 file changed, 103 insertions(+), 3 deletions(-)

-- 
2.26.2

Re: [PATCH v5] kbuild: add variables for compression tools

[Denis Efremov]

On 6/8/20 7:59 AM, Masahiro Yamada wrote:
> On Mon, Jun 8, 2020 at 10:30 AM Guenter Roeck  wrote:
>>
>> Hi,
>>
>> On Fri, Jun 05, 2020 at 10:39:55AM +0300, Denis Efremov wrote:
>>> Allow user to use alternative implementations of compression tools,
>>> such as pigz, pbzip2, pxz. For example, multi-threaded tools to
>>> speed up the build:
>>> $ make GZIP=pigz BZIP2=pbzip2
>>>
>>> Variables _GZIP, _BZIP2, _LZOP are used internally because original env
>>> vars are reserved by the tools. The use of GZIP in gzip tool is obsolete
>>> since 2015. However, alternative implementations (e.g., pigz) still rely
>>> on it. BZIP2, BZIP, LZOP vars are not obsolescent.
>>>
>>
>> When building mips:defconfig, this patch results in:
>>
>> Building mips:defconfig ... failed
>> --
>> Error log:
>> /bin/sh: -n: command not found
>> make[3]: *** [kernel/config_data.gz] Error 127
>> make[3]: *** Deleting file 'kernel/config_data.gz'
>> make[3]: *** Waiting for unfinished jobs
>> make[2]: *** [kernel] Error 2
>> make[2]: *** Waiting for unfinished jobs
>> make[1]: *** [autoksyms_recursive] Error 2
>> make: *** [__sub-make] Error 2
>>
>> Reverting this patch fixes the problem. Bisect log is attached.
>>
>> Guenter
>

I tried to reproduce it with cross-compilation on Fedora32.
$ export ARCH=mips
$ export CROSS_COMPILE=mips64-linux-gnu-
$ make defconfig
$ make -j12

And the kernel builds successfully. Could you please provide details about your
compilation steps and environment, esp. what "env | grep ZIP" shows,
"gzip --version", "sh --version", "bash --version"? This will be very helpful.

Additionally:
$ make GZIP=gzip -j12 # works
$ make GZIP=pigz -j12 # works
$ make GZIP=nosuchcommand -j12 # fails, as expected
 
> 
> Agh, this is because of CONFIG_TRIM_UNUSED_KSYMS.
> 

Hmm, it somehow works on my machine. But yes, this call looks like a problem
for these env vars:

autoksyms_recursive: descend modules.order  
 
$(Q)$(CONFIG_SHELL) $(srctree)/scripts/adjust_autoksyms.sh \
 
  "$(MAKE) -f $(srctree)/Makefile vmlinux" 

> Also, the distro package builds are broken
> e.g.  make GZIP=gzip bindeb-pkg
> 

Yes, thanks.

> Denis,
> 
> I think we should go back to the original
> KGZIP, KBZIP2, KLZOP.
> 

Given that the original patch is already in the Linus tree,
I've sent a hotfix. Commit message is not perfect, because I
didn't have enough time to deeply debug it. I just hope that
the original patch didn't broke too many builds. Maybe later
I will try to prepare a patch with GZIP again when I will fully
debug these corner cases.

Thanks,
Denis

[PATCH] kbuild: fix broken builds because of GZIP,BZIP2,LZOP variables

[Denis Efremov]

Redefine GZIP, BZIP2, LZOP variables as KGZIP, KBZIP2, KLZOP resp.
GZIP, BZIP2, LZOP env variables are reserved by the tools. The original
attempt to redefine them internally doesn't work in makefiles/scripts
intercall scenarios, e.g., "make GZIP=gzip bindeb-pkg" and results in
broken builds. There can be other broken build commands because of this,
so the universal solution is to use non-reserved env variables for the
compression tools.

Fixes: 8dfb61dcbace ("kbuild: add variables for compression tools")
Signed-off-by: Denis Efremov 
---
 Makefile  | 24 +---
 arch/arm/boot/deflate_xip_data.sh |  2 +-
 arch/ia64/Makefile|  2 +-
 arch/m68k/Makefile|  8 
 arch/parisc/Makefile  |  2 +-
 scripts/Makefile.lib  |  6 +++---
 scripts/Makefile.package  |  6 +++---
 scripts/package/buildtar  |  4 ++--
 8 files changed, 20 insertions(+), 34 deletions(-)

diff --git a/Makefile b/Makefile
index 839f9fee22cb..e43d193bb3b2 100644
--- a/Makefile
+++ b/Makefile
@@ -458,27 +458,13 @@ PYTHON= python
 PYTHON3= python3
 CHECK  = sparse
 BASH   = bash
-GZIP   = gzip
-BZIP2  = bzip2
-LZOP   = lzop
+KGZIP  = gzip
+KBZIP2 = bzip2
+KLZOP  = lzop
 LZMA   = lzma
 LZ4= lz4c
 XZ = xz
 
-# GZIP, BZIP2, LZOP env vars are used by the tools. Support them as the command
-# line interface, but use _GZIP, _BZIP2, _LZOP internally.
-_GZIP  := $(GZIP)
-_BZIP2 := $(BZIP2)
-_LZOP  := $(LZOP)
-
-# Reset GZIP, BZIP2, LZOP in this Makefile
-override GZIP=
-override BZIP2=
-override LZOP=
-
-# Reset GZIP, BZIP2, LZOP in recursive invocations
-MAKEOVERRIDES += GZIP= BZIP2= LZOP=
-
 CHECKFLAGS := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
  -Wbitwise -Wno-return-void -Wno-unknown-attribute $(CF)
 NOSTDINC_FLAGS :=
@@ -526,7 +512,7 @@ CLANG_FLAGS :=
 export ARCH SRCARCH CONFIG_SHELL BASH HOSTCC KBUILD_HOSTCFLAGS CROSS_COMPILE 
LD CC
 export CPP AR NM STRIP OBJCOPY OBJDUMP OBJSIZE READELF PAHOLE LEX YACC AWK 
INSTALLKERNEL
 export PERL PYTHON PYTHON3 CHECK CHECKFLAGS MAKE UTS_MACHINE HOSTCXX
-export _GZIP _BZIP2 _LZOP LZMA LZ4 XZ
+export KGZIP KBZIP2 KLZOP LZMA LZ4 XZ
 export KBUILD_HOSTCXXFLAGS KBUILD_HOSTLDFLAGS KBUILD_HOSTLDLIBS LDFLAGS_MODULE
 
 export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS KBUILD_LDFLAGS
@@ -1047,7 +1033,7 @@ export mod_strip_cmd
 mod_compress_cmd = true
 ifdef CONFIG_MODULE_COMPRESS
   ifdef CONFIG_MODULE_COMPRESS_GZIP
-mod_compress_cmd = $(_GZIP) -n -f
+mod_compress_cmd = $(KGZIP) -n -f
   endif # CONFIG_MODULE_COMPRESS_GZIP
   ifdef CONFIG_MODULE_COMPRESS_XZ
 mod_compress_cmd = $(XZ) -f
diff --git a/arch/arm/boot/deflate_xip_data.sh 
b/arch/arm/boot/deflate_xip_data.sh
index 739f0464321e..304495c3c2c5 100755
--- a/arch/arm/boot/deflate_xip_data.sh
+++ b/arch/arm/boot/deflate_xip_data.sh
@@ -56,7 +56,7 @@ trap 'rm -f "$XIPIMAGE.tmp"; exit 1' 1 2 3
 # substitute the data section by a compressed version
 $DD if="$XIPIMAGE" count=$data_start iflag=count_bytes of="$XIPIMAGE.tmp"
 $DD if="$XIPIMAGE"  skip=$data_start iflag=skip_bytes |
-$_GZIP -9 >> "$XIPIMAGE.tmp"
+$KGZIP -9 >> "$XIPIMAGE.tmp"
 
 # replace kernel binary
 mv -f "$XIPIMAGE.tmp" "$XIPIMAGE"
diff --git a/arch/ia64/Makefile b/arch/ia64/Makefile
index f817f3d5e758..2876a7df1b0a 100644
--- a/arch/ia64/Makefile
+++ b/arch/ia64/Makefile
@@ -40,7 +40,7 @@ $(error Sorry, you need a newer version of the assember, one 
that is built from
 endif
 
 quiet_cmd_gzip = GZIP$@
-cmd_gzip = cat $(real-prereqs) | $(_GZIP) -n -f -9 > $@
+cmd_gzip = cat $(real-prereqs) | $(KGZIP) -n -f -9 > $@
 
 quiet_cmd_objcopy = OBJCOPY $@
 cmd_objcopy = $(OBJCOPY) $(OBJCOPYFLAGS) $(OBJCOPYFLAGS_$(@F)) $< $@
diff --git a/arch/m68k/Makefile b/arch/m68k/Makefile
index ce6db5e5a5a3..0415d28dbe4f 100644
--- a/arch/m68k/Makefile
+++ b/arch/m68k/Makefile
@@ -135,10 +135,10 @@ vmlinux.gz: vmlinux
 ifndef CONFIG_KGDB
cp vmlinux vmlinux.tmp
$(STRIP) vmlinux.tmp
-   $(_GZIP) -9c vmlinux.tmp >vmlinux.gz
+   $(KGZIP) -9c vmlinux.tmp >vmlinux.gz
rm vmlinux.tmp
 else
-   $(_GZIP) -9c vmlinux >vmlinux.gz
+   $(KGZIP) -9c vmlinux >vmlinux.gz
 endif
 
 bzImage: vmlinux.bz2
@@ -148,10 +148,10 @@ vmlinux.bz2: vmlinux
 ifndef CONFIG_KGDB
cp vmlinux vmlinux.tmp
$(STRIP) vmlinux.tmp
-   $(_BZIP2) -1c vmlinux.tmp >vmlinux.bz2
+   $(KBZIP2) -1c vmlinux.tmp >vmlinux.bz2
rm vmlinux.tmp
 else
-   $(_BZIP2) -1c vmlinux >vmlinux.bz2
+   $(KBZIP2) -1c vmlinux >vmlinux.bz2
 endif
 
 archclean:
diff --git a/arch/parisc/Makefile b/arch/parisc/Makefile
index 182a5bca3e2c..5140c6022

Re: [Cocci] [PATCH 1/2] Coccinelle: extend memdup_user transformation with GFP_USER

[Denis Efremov]

On 6/6/20 11:24 AM, Julia Lawall wrote:
> 
> 
> On Sat, 30 May 2020, Denis Efremov wrote:
> 
>> Match GFP_USER allocations with memdup_user.cocci rule.
>> Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
>> memdup_user() from GFP_KERNEL to GFP_USER. In most cases it is still
>> a good idea to use memdup_user() for GFP_KERNEL allocations. The
>> motivation behind altering memdup_user() to GFP_USER is here:
>> https://lkml.org/lkml/2018/1/6/333
> 
> Should the rule somehow document the cases in which memdup_user should now
> not be used?
> 
> julia
> 
> 
>>
>> Signed-off-by: Denis Efremov 
>> ---
>>  scripts/coccinelle/api/memdup_user.cocci | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/coccinelle/api/memdup_user.cocci 
>> b/scripts/coccinelle/api/memdup_user.cocci
>> index c809ab10bbce..49f487e6a5c8 100644
>> --- a/scripts/coccinelle/api/memdup_user.cocci
>> +++ b/scripts/coccinelle/api/memdup_user.cocci
>> @@ -20,7 +20,7 @@ expression from,to,size;
>>  identifier l1,l2;
>>  @@
>>
>> --  to = \(kmalloc\|kzalloc\)(size,GFP_KERNEL);
>> +-  to = \(kmalloc\|kzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));

Actually, we can add optional __GFP_NOWARN here to match such cases as:
GFP_KERNEL | __GFP_NOWARN

However, I don't know how to express it in elegant way. Something like?
(
-  to = \(kmalloc\|kzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
|
-  to = \(kmalloc\|kzalloc\)(size, GFP_KERNEL|__GFP_NOWARN);
|
-  to = \(kmalloc\|kzalloc\)(size, GFP_USER|__GFP_NOWARN);
)

Thanks,
Denis

Re: [Cocci] [PATCH 1/2] Coccinelle: extend memdup_user transformation with GFP_USER

[Denis Efremov]

On 6/6/20 11:24 AM, Julia Lawall wrote:
> 
> 
> On Sat, 30 May 2020, Denis Efremov wrote:
> 
>> Match GFP_USER allocations with memdup_user.cocci rule.
>> Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
>> memdup_user() from GFP_KERNEL to GFP_USER. In most cases it is still
>> a good idea to use memdup_user() for GFP_KERNEL allocations. The
>> motivation behind altering memdup_user() to GFP_USER is here:
>> https://lkml.org/lkml/2018/1/6/333
> 
> Should the rule somehow document the cases in which memdup_user should now
> not be used?

As for now, I can't provide a counterexample. GPF_USER is more permissive than
GFP_KERNEL. It's completely ok to use GPF_USER with copy_from_user. Given that
memdup_user() was "silently" switched to GPF_USER from GPF_KERNEL with no 
callside
fixes, I think it's ok to recommend to use memdup_user for GPF_KERNEL matches 
with
no additional restrictions.

Thanks,
Denis

Re: [Cocci] [PATCH] coccinelle: api: add kvfree script

[Denis Efremov]

On 6/5/20 11:51 PM, Julia Lawall wrote:
> Is there a strong reason for putting the choice rule first?  It may make
> things somewhat slower than necessary, if it matches in many places,
> because the opportunity rule will have to detect that it doesn't care
> about all of those places.

No, I didn't know that order of rules matters. I just checked it, my PC
shows no difference in exec time if I swap these rules.

"choice" doesn't check the size. "opportunity" is more strict.
The motivation for adding 2 rules is that we could recommend to use
kvmalloc* only when there is a size condition. At the same time, we
should skip all if (...) {kmalloc()} else {vmalloc()},
res = kmalloc() if (!res) {vmalloc()} cases as false positives.

It seems that it's not possible to use subexpression rule
"expression size <= choice.E" in this case.

> Also, there is no need to exceed 80 characters here.  You can put a
> newline in the middle of a \( ... \)

Ok, I will fix it in v2 after all comments/suggestions.

Thanks,
Denis

[PATCH] coccinelle: api: add kvfree script

[Denis Efremov]

Check that alloc and free types of functions match each other.

Signed-off-by: Denis Efremov 
---
List of patches to stable:
- https://lkml.org/lkml/2020/6/1/713
- https://lkml.org/lkml/2020/6/5/200
- https://lkml.org/lkml/2020/6/5/838
- https://lkml.org/lkml/2020/6/5/887

Other patches:
- https://lkml.org/lkml/2020/6/1/701
- https://lkml.org/lkml/2020/6/5/839
- https://lkml.org/lkml/2020/6/5/864
- https://lkml.org/lkml/2020/6/5/865
- https://lkml.org/lkml/2020/6/5/895
- https://lkml.org/lkml/2020/6/5/901

There is a false positive that I can't beat:
fs/btrfs/send.c:1119:11-12: WARNING: kmalloc is used to allocate
this memory at line 1036

 scripts/coccinelle/api/kvfree.cocci | 196 
 1 file changed, 196 insertions(+)
 create mode 100644 scripts/coccinelle/api/kvfree.cocci

diff --git a/scripts/coccinelle/api/kvfree.cocci 
b/scripts/coccinelle/api/kvfree.cocci
new file mode 100644
index ..e3fa3d0fd2fd
--- /dev/null
+++ b/scripts/coccinelle/api/kvfree.cocci
@@ -0,0 +1,196 @@
+// 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
+
+
+@choice@
+expression E, E1;
+position kok, vok;
+@@
+
+(
+  if (...) {
+...
+E = 
\(kmalloc@kok\|kzalloc@kok\|krealloc@kok\|kcalloc@kok\|kmalloc_node@kok\|kzalloc_node@kok\|kmalloc_array@kok\|kmalloc_array_node@kok\|kcalloc_node@kok\)(...)
+...
+  } else {
+...
+E = 
\(vmalloc@vok\|vzalloc@vok\|vmalloc_user@vok\|vmalloc_node@vok\|vzalloc_node@vok\|vmalloc_exec@vok\|vmalloc_32@vok\|vmalloc_32_user@vok\|__vmalloc@vok\|__vmalloc_node_range@vok\|__vmalloc_node@vok\)(...)
+...
+  }
+|
+  E = 
\(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(...)
+  ... when != E = E1
+  when any
+  if (\(!E\|E == NULL\)) {
+...
+E = 
\(vmalloc@vok\|vzalloc@vok\|vmalloc_user@vok\|vmalloc_node@vok\|vzalloc_node@vok\|vmalloc_exec@vok\|vmalloc_32@vok\|vmalloc_32_user@vok\|__vmalloc@vok\|__vmalloc_node_range@vok\|__vmalloc_node@vok\)(...)
+...
+  }
+)
+
+// exclude mm/vmalloc.c because of kvmalloc* definitions
+@opportunity depends on !patch && !(file in "mm/vmalloc.c")@
+expression E, E1, size;
+position p;
+@@
+
+(
+* if (\(size <= E1\|size < E1\|size = E1\|size > E1\) || ...)@p {
+...
+E = 
\(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(...,
 size, ...)
+...
+  } else {
+...
+E = 
\(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\)(...,
 size, ...)
+...
+  }
+|
+  E = 
\(kmalloc\|kzalloc\|krealloc\|kcalloc\|kmalloc_node\|kzalloc_node\|kmalloc_array\|kmalloc_array_node\|kcalloc_node\)(...,
 size, ...)
+  ... when != E = E1
+  when != size = E1
+  when any
+* if (\(!E\|E == NULL\))@p {
+...
+E = 
\(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\)(...,
 size, ...)
+...
+  }
+)
+
+@vfree depends on !patch@
+expression E;
+position k != choice.kok;
+position p;
+@@
+
+* E = 
\(kmalloc@k\|kzalloc@k\|krealloc@k\|kcalloc@k\|kmalloc_node@k\|kzalloc_node@k\|kmalloc_array@k\|kmalloc_array_node@k\|kcalloc_node@k\)(...)
+  ... when != if (...) { ... E = 
\(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\|kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...);
 ... }
+  when != is_vmalloc_addr(E)
+  when any
+* \(vfree\|vfree_atomic\|kvfree\)(E)@p
+
+@pvfree depends on patch exists@
+expression E;
+position k != choice.kok;
+@@
+
+  E = 
\(kmalloc@k\|kzalloc@k\|krealloc@k\|kcalloc@k\|kmalloc_node@k\|kzalloc_node@k\|kmalloc_array@k\|kmalloc_array_node@k\|kcalloc_node@k\)(...)
+  ... when != if (...) { ... E = 
\(vmalloc\|vzalloc\|vmalloc_user\|vmalloc_node\|vzalloc_node\|vmalloc_exec\|vmalloc_32\|vmalloc_32_user\|__vmalloc\|__vmalloc_node_range\|__vmalloc_node\|kvmalloc\|kvzalloc\|kvcalloc\|kvzalloc_node\|kvmalloc_node\|kvmalloc_array\)(...);
 ... }
+  when != is_vmalloc_addr(E)
+  when any
+- \(vfree\|vfree_atomic\|kvfree\)(E)
++ kfree(E)
+
+@kfree depends on !patch@
+expression E;
+position v != choice.vok;
+position p;
+@@
+
+* E = 
\(vmalloc@v\|vzalloc@v\|vmalloc_user@v\|vmalloc_node@v\|vzalloc_node@v\|vmalloc_exec@v\|vmalloc_32@v\|vmalloc_32_user@v\|__vmalloc@v\|__vmalloc_node_range@v\|__vmalloc_node@v\)(...)
+  ... when != !is_vmalloc_addr(E)
+  when any
+* 

[PATCH] net/mlx5: Use kfree(ft->g) in arfs_create_groups()

[Denis Efremov]

Use kfree() instead of kvfree() on ft->g in arfs_create_groups() because
the memory is allocated with kcalloc().

Signed-off-by: Denis Efremov 
---
 drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c 
b/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
index 014639ea06e3..c4c9d6cda7e6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
@@ -220,7 +220,7 @@ static int arfs_create_groups(struct mlx5e_flow_table *ft,
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] cxgb4: Use kfree() instead kvfree() where appropriate

[Denis Efremov]

Use kfree(buf) in blocked_fl_read() because the memory is allocated with
kzalloc(). Use kfree(t) in blocked_fl_write() because the memory is
allocated with kcalloc().

Signed-off-by: Denis Efremov 
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c 
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
index 41315712deb8..828499256004 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
@@ -3357,7 +3357,7 @@ static ssize_t blocked_fl_read(struct file *filp, char 
__user *ubuf,
   adap->sge.egr_sz, adap->sge.blocked_fl);
len += sprintf(buf + len, "\n");
size = simple_read_from_buffer(ubuf, count, ppos, buf, len);
-   kvfree(buf);
+   kfree(buf);
return size;
 }
 
@@ -3374,12 +3374,12 @@ static ssize_t blocked_fl_write(struct file *filp, 
const char __user *ubuf,
 
err = bitmap_parse_user(ubuf, count, t, adap->sge.egr_sz);
if (err) {
-   kvfree(t);
+   kfree(t);
return err;
}
 
bitmap_copy(adap->sge.blocked_fl, t, adap->sge.egr_sz);
-   kvfree(t);
+   kfree(t);
return count;
 }
 
-- 
2.26.2

[PATCH] drm/panfrost: Use kvfree() to free bo->sgts in panfrost_mmu_map_fault_addr()

[Denis Efremov]

Use kvfree() to free bo->sgts, because the memory is allocated with
kvmalloc_array().

Fixes: 187d2929206e ("drm/panfrost: Add support for GPU heap allocations")
Cc: sta...@vger.kernel.org
Signed-off-by: Denis Efremov 
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c 
b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index ed28aeba6d59..3c8ae7411c80 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -486,7 +486,7 @@ static int panfrost_mmu_map_fault_addr(struct 
panfrost_device *pfdev, int as,
pages = kvmalloc_array(bo->base.base.size >> PAGE_SHIFT,
   sizeof(struct page *), GFP_KERNEL | 
__GFP_ZERO);
if (!pages) {
-   kfree(bo->sgts);
+   kvfree(bo->sgts);
bo->sgts = NULL;
mutex_unlock(>base.pages_lock);
ret = -ENOMEM;
-- 
2.26.2

[PATCH 1/2] f2fs: use kfree() to free super in read_raw_super_block()

[Denis Efremov]

Use kfree() instead of kvfree() to free super in
read_raw_super_block() because the memory is allocated with
kzalloc() in the function.

Fixes: 5222595d093e ("f2fs: use kvmalloc, if kmalloc is failed")
Signed-off-by: Denis Efremov 
---
 fs/f2fs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 8a9955902d84..9a3c8eba37e2 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3130,7 +3130,7 @@ static int read_raw_super_block(struct f2fs_sb_info *sbi,
 
/* No valid superblock */
if (!*raw_super)
-   kvfree(super);
+   kfree(super);
else
err = 0;
 
-- 
2.26.2

[PATCH 2/2] f2fs: use kfree() to free sbi in f2fs_fill_super()

[Denis Efremov]

Use kfree() instead of kvfree() to free sbi in
f2fs_fill_super() because the memory is allocated with
kzalloc() in the function.

Fixes: 5222595d093e ("f2fs: use kvmalloc, if kmalloc is failed")
Signed-off-by: Denis Efremov 
---
 fs/f2fs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 9a3c8eba37e2..39b644c7e9d4 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3812,7 +3812,7 @@ static int f2fs_fill_super(struct super_block *sb, void 
*data, int silent)
 free_sbi:
if (sbi->s_chksum_driver)
crypto_free_shash(sbi->s_chksum_driver);
-   kvfree(sbi);
+   kfree(sbi);
 
/* give only one another chance */
if (retry_cnt > 0 && skip_recovery) {
-- 
2.26.2

[PATCH 1/2] drm/amd/display: Use kvfree() to free coeff in build_regamma()

[Denis Efremov]

Use kvfree() instead of kfree() to free coeff in build_regamma()
because the memory is allocated with kvzalloc().

Fixes: e752058b8671 ("drm/amd/display: Optimize gamma calculations")
Cc: sta...@vger.kernel.org
Signed-off-by: Denis Efremov 
---
 drivers/gpu/drm/amd/display/modules/color/color_gamma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/modules/color/color_gamma.c 
b/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
index 9431b48aecb4..56bb1f9f77ce 100644
--- a/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
+++ b/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
@@ -843,7 +843,7 @@ static bool build_regamma(struct pwl_float_data_ex 
*rgb_regamma,
pow_buffer_ptr = -1; // reset back to no optimize
ret = true;
 release:
-   kfree(coeff);
+   kvfree(coeff);
return ret;
 }
 
-- 
2.26.2