linux-kernel - [RFC 0/1] New IDA API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20180326173037.11824-1-willy@infradead.org>
Date:   Mon, 26 Mar 2018 10:30:36 -0700
From:   Matthew Wilcox <willy@...radead.org>
To:     linux-kernel@...r.kernel.org
Cc:     Tejun Heo <tj@...nel.org>, Matthew Wilcox <mawilcox@...rosoft.com>
Subject: [RFC 0/1] New IDA API

From: Matthew Wilcox <mawilcox@...rosoft.com>

I have a tree locally with no more calls to ida_pre_get(),
ida_get_new_above(), ida_get_new() or ida_remove(), leaving us with only
calls to ida_simple_get() and ida_simple_remove().  With no 'complex'
versions, naming the functions ida_simple_* seems pointless.  I also
don't think that 'get' and 'remove' are antonyms.

The percpu_ida has the right names, in my opinion, 'alloc' and 'free'.
We're used to alloc and free from kmalloc/kfree and many other examples.
Looking through the existing users, ida_simple_get() is a little too
flexible for some users, so I included helper wrappers which allow
*most* users to just call ida_alloc(), while those that want a bounded
end can call ida_alloc_min() or ida_alloc_max().  ida_alloc_range() is
available for those who want to specify both ends of the range.

The most controversial thing is how to specify the upper bound on the
ID to allocate.  The current ida_simple_get() specifies an exclusive
bound (ie one higher than the maximum ID to return), while
ida_alloc_max() / ida_alloc_range() specify an inclusive bound (the
maximum ID to return).  Both styles of bound specification have their
adherents within the kernel, and current users seem about equally split
whether they would prefer 'end' or 'max':

drivers/block/virtio_blk.c:     err = ida_simple_get(&vd_index_ida, 0, minor_to_index(1 << MINORBITS),
drivers/dax/super.c:    minor = ida_simple_get(&dax_minor_ida, 0, MINORMASK+1, GFP_KERNEL);

Some have been confused by the current convention:

drivers/fsi/fsi-core.c: master->idx = ida_simple_get(&master_ida, 0, INT_MAX, GFP_KERNEL);
(pretty sure they'd be OK with returning INT_MAX)

One aspect I particularly like about specifying an inclusive rather than
exclusive bound is that this is not an uncommon pattern:
	id = ida_simple_get(&rtc_ida, of_id, of_id + 1, GFP_KERNEL);

and transforming that to
	id = ida_alloc_range(&rtc_ida, of_id, of_id, GFP_KERNEL);
makes more sense.  Also, there is a bug in the current implementation
of ida_simple_get where the above call would not just fail to allocate
an ID but actually BUG() if of_id happened to be INT_MAX.

I'm still musing whether the API should be expressed in terms of int,
or whether 64-bit systems might appreciate having the extra space of
'long'.  The underlying data structure supports 'unsigned long', but an
API that uses that is much harder to use and in the absence of any users,
I'm not going to add it.  Converting from int to long would hardly
change the interface, but it would lead to a situation where you could
allocate IDs on 64-bit systems that could never be allocated on 32-bit
systems.

Matthew Wilcox (1):
  ida: Add new API

 include/linux/idr.h | 59 +++++++++++++++++++++++++++++++++++++++++---
 lib/idr.c           | 70 ++++++++++++++++++++++++-----------------------------
 2 files changed, 87 insertions(+), 42 deletions(-)

-- 
2.16.2