* [PATCH 13/13] security/integrity/ima: converts stats to seqnum_ops
2020-11-10 19:53 [PATCH 00/13] Introduce seqnum_ops Shuah Khan
@ 2020-11-10 19:53 ` Shuah Khan
2020-11-11 8:51 ` kernel test robot
2020-11-10 20:44 ` [PATCH 00/13] Introduce seqnum_ops Alan Stern
2020-11-11 4:33 ` Matthew Wilcox
2 siblings, 1 reply; 8+ messages in thread
From: Shuah Khan @ 2020-11-10 19:53 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, jmorris, serge, gregkh, keescook, peterz
Cc: Shuah Khan, linux-security-module, linux-integrity, linux-kernel
seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.
seqnum32 variables wrap around to INT_MIN when it overflows and
should not be used to guard resource lifetimes, device usage and
open counts that control state changes, and pm states.
atomic_t variables used for eima_htable.violations and number of stored
measurements and ios_threshold are atomic counters, and violations is
only an idicator and can overflow. No chane to the behavior with this
change.
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
security/integrity/ima/ima.h | 5 +++--
security/integrity/ima/ima_api.c | 2 +-
security/integrity/ima/ima_fs.c | 4 ++--
security/integrity/ima/ima_queue.c | 7 ++++---
4 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 6ebefec616e4..55fe1d14c67a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -21,6 +21,7 @@
#include <linux/tpm.h>
#include <linux/audit.h>
#include <crypto/hash_info.h>
+#include <linux/seqnum_ops.h>
#include "../integrity.h"
@@ -174,8 +175,8 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
extern spinlock_t ima_queue_lock;
struct ima_h_table {
- atomic_long_t len; /* number of stored measurements in the list */
- atomic_long_t violations;
+ struct seqnum64 len; /* number of stored measurements in the list */
+ struct seqnum64 violations;
struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
};
extern struct ima_h_table ima_htable;
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 4f39fb93f278..b1a203435698 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -144,7 +144,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
int result;
/* can overflow, only indicator */
- atomic_long_inc(&ima_htable.violations);
+ seqnum64_inc(&ima_htable.violations);
result = ima_alloc_init_template(&event_data, &entry, NULL);
if (result < 0) {
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index ea8ff8a07b36..03a78b445052 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -39,12 +39,12 @@ __setup("ima_canonical_fmt", default_canonical_fmt_setup);
static int valid_policy = 1;
static ssize_t ima_show_htable_value(char __user *buf, size_t count,
- loff_t *ppos, atomic_long_t *val)
+ loff_t *ppos, struct seqnum64 *val)
{
char tmpbuf[32]; /* greater than largest 'long' string value */
ssize_t len;
- len = scnprintf(tmpbuf, sizeof(tmpbuf), "%li\n", atomic_long_read(val));
+ len = scnprintf(tmpbuf, sizeof(tmpbuf), "%lli\n", seqnum64_read(val));
return simple_read_from_buffer(buf, count, ppos, tmpbuf, len);
}
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index c096ef8945c7..87db50dd1721 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -17,6 +17,7 @@
#include <linux/rculist.h>
#include <linux/slab.h>
+#include <linux/seqnum_ops.h>
#include "ima.h"
#define AUDIT_CAUSE_LEN_MAX 32
@@ -33,8 +34,8 @@ static unsigned long binary_runtime_size = ULONG_MAX;
/* key: inode (before secure-hashing a file) */
struct ima_h_table ima_htable = {
- .len = ATOMIC_LONG_INIT(0),
- .violations = ATOMIC_LONG_INIT(0),
+ .len = SEQNUM_INIT(0),
+ .violations = SEQNUM_INIT(0),
.queue[0 ... IMA_MEASURE_HTABLE_SIZE - 1] = HLIST_HEAD_INIT
};
@@ -106,7 +107,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry,
INIT_LIST_HEAD(&qe->later);
list_add_tail_rcu(&qe->later, &ima_measurements);
- atomic_long_inc(&ima_htable.len);
+ seqnum64_inc(&ima_htable.len);
if (update_htable) {
key = ima_hash_key(entry->digests[ima_hash_algo_idx].digest);
hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
--
2.27.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 13/13] security/integrity/ima: converts stats to seqnum_ops
2020-11-10 19:53 ` [PATCH 13/13] security/integrity/ima: converts stats to seqnum_ops Shuah Khan
@ 2020-11-11 8:51 ` kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2020-11-11 8:51 UTC (permalink / raw)
To: Shuah Khan, zohar, dmitry.kasatkin, jmorris, serge, gregkh,
keescook, peterz
Cc: kbuild-all, Shuah Khan, linux-security-module, linux-integrity
[-- Attachment #1: Type: text/plain, Size: 4013 bytes --]
Hi Shuah,
I love your patch! Perhaps something to improve:
[auto build test WARNING on staging/staging-testing]
[also build test WARNING on integrity/next-integrity char-misc/char-misc-testing usb/usb-testing linus/master v5.10-rc3 next-20201110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Shuah-Khan/Introduce-seqnum_ops/20201111-035753
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git f4acd33c446b2ba97f1552a4da90050109d01ca7
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/4124aef613b0e30b7da08aaec750983854e1ca5a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Shuah-Khan/Introduce-seqnum_ops/20201111-035753
git checkout 4124aef613b0e30b7da08aaec750983854e1ca5a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from security/integrity/ima/ima_fs.c:25:
security/integrity/ima/ima.h:178:18: error: field 'len' has incomplete type
178 | struct seqnum64 len; /* number of stored measurements in the list */
| ^~~
security/integrity/ima/ima.h:179:18: error: field 'violations' has incomplete type
179 | struct seqnum64 violations;
| ^~~~~~~~~~
security/integrity/ima/ima_fs.c: In function 'ima_show_htable_value':
security/integrity/ima/ima_fs.c:47:52: error: implicit declaration of function 'seqnum64_read'; did you mean 'seqnum32_read'? [-Werror=implicit-function-declaration]
47 | len = scnprintf(tmpbuf, sizeof(tmpbuf), "%lli\n", seqnum64_read(val));
| ^~~~~~~~~~~~~
| seqnum32_read
>> security/integrity/ima/ima_fs.c:47:46: warning: format '%lli' expects argument of type 'long long int', but argument 4 has type 'int' [-Wformat=]
47 | len = scnprintf(tmpbuf, sizeof(tmpbuf), "%lli\n", seqnum64_read(val));
| ~~~^ ~~~~~~~~~~~~~~~~~~
| | |
| | int
| long long int
| %i
security/integrity/ima/ima_fs.c: In function 'ima_show_htable_violations':
security/integrity/ima/ima_fs.c:56:1: error: control reaches end of non-void function [-Werror=return-type]
56 | }
| ^
security/integrity/ima/ima_fs.c: In function 'ima_show_measurements_count':
security/integrity/ima/ima_fs.c:69:1: error: control reaches end of non-void function [-Werror=return-type]
69 | }
| ^
cc1: some warnings being treated as errors
vim +47 security/integrity/ima/ima_fs.c
40
41 static ssize_t ima_show_htable_value(char __user *buf, size_t count,
42 loff_t *ppos, struct seqnum64 *val)
43 {
44 char tmpbuf[32]; /* greater than largest 'long' string value */
45 ssize_t len;
46
> 47 len = scnprintf(tmpbuf, sizeof(tmpbuf), "%lli\n", seqnum64_read(val));
48 return simple_read_from_buffer(buf, count, ppos, tmpbuf, len);
49 }
50
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 53507 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 00/13] Introduce seqnum_ops
2020-11-10 19:53 [PATCH 00/13] Introduce seqnum_ops Shuah Khan
2020-11-10 19:53 ` [PATCH 13/13] security/integrity/ima: converts stats to seqnum_ops Shuah Khan
@ 2020-11-10 20:44 ` Alan Stern
2020-11-10 22:42 ` Shuah Khan
2020-11-11 4:33 ` Matthew Wilcox
2 siblings, 1 reply; 8+ messages in thread
From: Alan Stern @ 2020-11-10 20:44 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, keescook, gregkh, peterz, rafael, lenb, james.morse,
tony.luck, bp, minyard, arnd, mchehab, rric, valentina.manea.m,
shuah, zohar, dmitry.kasatkin, jmorris, serge, linux-doc,
linux-kernel, linux-kselftest, linux-acpi, openipmi-developer,
linux-edac, linux-usb, linux-integrity, linux-security-module
On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
> There are a number of atomic_t usages in the kernel where atomic_t api
> is used strictly for counting sequence numbers and other statistical
> counters and not for managing object lifetime.
>
> The purpose of these Sequence Number Ops is to clearly differentiate
> atomic_t counter usages from atomic_t usages that guard object lifetimes,
> hence prone to overflow and underflow errors.
>
> The atomic_t api provides a wide range of atomic operations as a base
> api to implement atomic counters, bitops, spinlock interfaces. The usages
> also evolved into being used for resource lifetimes and state management.
> The refcount_t api was introduced to address resource lifetime problems
> related to atomic_t wrapping. There is a large overlap between the
> atomic_t api used for resource lifetimes and just counters, stats, and
> sequence numbers. It has become difficult to differentiate between the
> atomic_t usages that should be converted to refcount_t and the ones that
> can be left alone. Introducing seqnum_ops to wrap the usages that are
> stats, counters, sequence numbers makes it easier for tools that scan
> for underflow and overflow on atomic_t usages to detect overflow and
> underflows to scan just the cases that are prone to errors.
>
> Sequence Number api provides interfaces for simple atomic_t counter usages
> that just count, and don't guard resource lifetimes. The seqnum_ops are
> built on top of atomic_t api, providing a smaller subset of atomic_t
> interfaces necessary to support atomic_t usages as simple counters.
> This api has init/set/inc/dec/read and doesn't support any other atomic_t
> ops with the intent to restrict the use of these interfaces as simple
> counting usages.
>
> Sequence Numbers wrap around to INT_MIN when it overflows and should not
> be used to guard resource lifetimes, device usage and open counts that
> control state changes, and pm states. Overflowing to INT_MIN is consistent
> with the atomic_t api, which it is built on top of.
If Sequence Numbers are subject to wraparound then they aren't reliable.
Given that they aren't reliable, why use atomic instructions at all?
Why not just use plain regular integers with READ_ONCE and WRITE_ONCE?
Alan Stern
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 00/13] Introduce seqnum_ops
2020-11-10 20:44 ` [PATCH 00/13] Introduce seqnum_ops Alan Stern
@ 2020-11-10 22:42 ` Shuah Khan
0 siblings, 0 replies; 8+ messages in thread
From: Shuah Khan @ 2020-11-10 22:42 UTC (permalink / raw)
To: Alan Stern
Cc: corbet, keescook, gregkh, peterz, rafael, lenb, james.morse,
tony.luck, bp, minyard, arnd, mchehab, rric, valentina.manea.m,
shuah, zohar, dmitry.kasatkin, jmorris, serge, linux-doc,
linux-kernel, linux-kselftest, linux-acpi, openipmi-developer,
linux-edac, linux-usb, linux-integrity, linux-security-module
On 11/10/20 1:44 PM, Alan Stern wrote:
> On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
>> There are a number of atomic_t usages in the kernel where atomic_t api
>> is used strictly for counting sequence numbers and other statistical
>> counters and not for managing object lifetime.
>>
>> The purpose of these Sequence Number Ops is to clearly differentiate
>> atomic_t counter usages from atomic_t usages that guard object lifetimes,
>> hence prone to overflow and underflow errors.
>>
>> The atomic_t api provides a wide range of atomic operations as a base
>> api to implement atomic counters, bitops, spinlock interfaces. The usages
>> also evolved into being used for resource lifetimes and state management.
>> The refcount_t api was introduced to address resource lifetime problems
>> related to atomic_t wrapping. There is a large overlap between the
>> atomic_t api used for resource lifetimes and just counters, stats, and
>> sequence numbers. It has become difficult to differentiate between the
>> atomic_t usages that should be converted to refcount_t and the ones that
>> can be left alone. Introducing seqnum_ops to wrap the usages that are
>> stats, counters, sequence numbers makes it easier for tools that scan
>> for underflow and overflow on atomic_t usages to detect overflow and
>> underflows to scan just the cases that are prone to errors.
>>
>> Sequence Number api provides interfaces for simple atomic_t counter usages
>> that just count, and don't guard resource lifetimes. The seqnum_ops are
>> built on top of atomic_t api, providing a smaller subset of atomic_t
>> interfaces necessary to support atomic_t usages as simple counters.
>> This api has init/set/inc/dec/read and doesn't support any other atomic_t
>> ops with the intent to restrict the use of these interfaces as simple
>> counting usages.
>>
>> Sequence Numbers wrap around to INT_MIN when it overflows and should not
>> be used to guard resource lifetimes, device usage and open counts that
>> control state changes, and pm states. Overflowing to INT_MIN is consistent
>> with the atomic_t api, which it is built on top of.
>
> If Sequence Numbers are subject to wraparound then they aren't reliable.
> Given that they aren't reliable, why use atomic instructions at all?
> Why not just use plain regular integers with READ_ONCE and WRITE_ONCE?
>
You still need atomic update for these numbers. The intent is to provide
atomic api for cases where the variable doesn't guard lifetimes and yet
needs atomic instructions.
Several such usages where atomic_t is used for up counting, also use
upper bounds. It is also an option to switch to seqnum64 to avoid
wrap around in case there is a concern.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 00/13] Introduce seqnum_ops
2020-11-10 19:53 [PATCH 00/13] Introduce seqnum_ops Shuah Khan
2020-11-10 19:53 ` [PATCH 13/13] security/integrity/ima: converts stats to seqnum_ops Shuah Khan
2020-11-10 20:44 ` [PATCH 00/13] Introduce seqnum_ops Alan Stern
@ 2020-11-11 4:33 ` Matthew Wilcox
2020-11-11 16:03 ` Shuah Khan
2 siblings, 1 reply; 8+ messages in thread
From: Matthew Wilcox @ 2020-11-11 4:33 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, keescook, gregkh, peterz, rafael, lenb, james.morse,
tony.luck, bp, minyard, arnd, mchehab, rric, valentina.manea.m,
shuah, zohar, dmitry.kasatkin, jmorris, serge, linux-doc,
linux-kernel, linux-kselftest, linux-acpi, openipmi-developer,
linux-edac, linux-usb, linux-integrity, linux-security-module
On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
> There are a number of atomic_t usages in the kernel where atomic_t api
> is used strictly for counting sequence numbers and other statistical
> counters and not for managing object lifetime.
We already have something in Linux called a sequence counter, and it's
different from this. ID counter? instance number? monotonic_t? stat_t?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 00/13] Introduce seqnum_ops
2020-11-11 4:33 ` Matthew Wilcox
@ 2020-11-11 16:03 ` Shuah Khan
2020-11-11 16:41 ` Matthew Wilcox
0 siblings, 1 reply; 8+ messages in thread
From: Shuah Khan @ 2020-11-11 16:03 UTC (permalink / raw)
To: Matthew Wilcox
Cc: corbet, keescook, gregkh, peterz, rafael, lenb, james.morse,
tony.luck, bp, minyard, arnd, mchehab, rric, valentina.manea.m,
shuah, zohar, dmitry.kasatkin, jmorris, serge, linux-doc,
linux-kernel, linux-kselftest, linux-acpi, openipmi-developer,
linux-edac, linux-usb, linux-integrity, linux-security-module,
skhan
On 11/10/20 9:33 PM, Matthew Wilcox wrote:
> On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
>> There are a number of atomic_t usages in the kernel where atomic_t api
>> is used strictly for counting sequence numbers and other statistical
>> counters and not for managing object lifetime.
>
> We already have something in Linux called a sequence counter, and it's
> different from this. ID counter? instance number? monotonic_t? stat_t?
>
No results for monotonic_t or stat_t. Can you give me a pointer to what
your referring to.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 00/13] Introduce seqnum_ops
2020-11-11 16:03 ` Shuah Khan
@ 2020-11-11 16:41 ` Matthew Wilcox
0 siblings, 0 replies; 8+ messages in thread
From: Matthew Wilcox @ 2020-11-11 16:41 UTC (permalink / raw)
To: Shuah Khan
Cc: corbet, keescook, gregkh, peterz, rafael, lenb, james.morse,
tony.luck, bp, minyard, arnd, mchehab, rric, valentina.manea.m,
shuah, zohar, dmitry.kasatkin, jmorris, serge, linux-doc,
linux-kernel, linux-kselftest, linux-acpi, openipmi-developer,
linux-edac, linux-usb, linux-integrity, linux-security-module
On Wed, Nov 11, 2020 at 09:03:20AM -0700, Shuah Khan wrote:
> On 11/10/20 9:33 PM, Matthew Wilcox wrote:
> > On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
> > > There are a number of atomic_t usages in the kernel where atomic_t api
> > > is used strictly for counting sequence numbers and other statistical
> > > counters and not for managing object lifetime.
> >
> > We already have something in Linux called a sequence counter, and it's
> > different from this. ID counter? instance number? monotonic_t? stat_t?
> >
>
> No results for monotonic_t or stat_t. Can you give me a pointer to what
> your referring to.
We have a seqcount_t. We need to call this something different.
maybe we should call it stat_t (and for that usage, stat_add() as well
as stat_inc() is a legitimate API to have).
^ permalink raw reply [flat|nested] 8+ messages in thread