On 10/05/2024 16:07, Przemek Kitszel wrote:
External email: Use caution opening links or attachments
please not that v4+ is already being discussed
On 5/8/24 13:33, Shay Drori wrote:
On 08/05/2024 12:34, Przemek Kitszel wrote:
// ...
+
+/* Auxiliary devices can share IRQs. Expose to user whether the
provided IRQ is
+ * shared or exclusive.
+ */
+static ssize_t auxiliary_irq_mode_show(struct device *dev,
+ struct device_attribute *attr,
char *buf)
+{
+ struct auxiliary_irq_info *info =
+ container_of(attr, struct auxiliary_irq_info,
sysfs_attr);
+
+ if (refcount_read(xa_load(&irqs, info->irq)) > 1)
I didn't checked if it is possible with current implementation, but
please imagine a scenario where user open()'s sysfs file, then triggers
operation to remove irq (to call auxiliary_irq_destroy()), and only then
read()'s sysfs contents, what results in nullptr dereference (xa_load()
returning NULL). Splitting the code into two if statements would resolve
this issue.
the first function in auxiliary_irq_destroy() is removing the sysfs.
I don't see how after that user can read() the sysfs...
Let me illustrate, but with my running kernel instead of your series:
# strace cat /sys/class/net/enp0s31f6/duplex 2>&1 | grep -e open -e read
yields (among others):
openat(AT_FDCWD, "/sys/class/net/enp0s31f6/duplex", O_RDONLY) = 3
read(3, "full\n", 131072) = 5
And now imagine that other, concurrent user app or any HW event triggers
this IRQ removal (resulting with xarray entry removed (!), likely sysfs
attr refcount dropped to 0 [A], so new open()s will be declined, but
that is irrelevant).
My assumption is that, until close()d, user is free to call read() on
fd received from openat(), but it's possible that xa_load() would return
NULL (because of [A] above).
+ return sysfs_emit(buf, "%s\n", "shared");
+ else
+ return sysfs_emit(buf, "%s\n", "exclusive");
+}
+
+static void auxiliary_irq_destroy(int irq)
+{
+ refcount_t *ref;
+
+ xa_lock(&irqs);
+ ref = xa_load(&irqs, irq);
+ if (refcount_dec_and_test(ref)) {
+ __xa_erase(&irqs, irq);
+ kfree(ref);
+ }
+ xa_unlock(&irqs);
+}
+
+static int auxiliary_irq_create(int irq)
+{
+ refcount_t *ref;
+ int ret = 0;
+
+ mutex_lock(&irqs_lock);
[1] xa_lock() instead ...
+ ref = xa_load(&irqs, irq);
+ if (ref && refcount_inc_not_zero(ref))
+ goto out;
`&& refcount_inc_not_zero()` here means: leak memory and wreak havoc on
saturation, instead the logic should be:
if (ref) {
refcount_inc(ref);
goto out;
}
<digression>
anyway allocating under a lock taken is not the best idea in general,
although xarray API somehow encourages this -
alternative is to
preallocate and free when not used, or some lock dance that will be easy
to get wrong - and that's the raison d'etre of xa_reserve() :)
I don't understand what you picture here?
Here I was digressing, sorry for not marking it clearly as that.
IMO xarray API need an extension to make this and similar use case
easier to code right. I will CC you ofc.
</digression>
xa_reserve() can drop the lock while allocating the xa_entry, so how it
will help?
+
+ ref = kzalloc(sizeof(*ref), GFP_KERNEL);
+ if (!ref) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ refcount_set(ref, 1);
+ ret = xa_insert(&irqs, irq, ref, GFP_KERNEL);
[2] ... then __xa_insert() here
__xa_insert() can drop the lock as well...
Thank you for pointing it to me.
Let's move future discussion on this series to your newer submissions.
thanks for the quick reviews :)
lets continue in the v4 series.
// ...
