s390/tape: iterator after loop end in tape_assign_minor?

s390/tape: iterator after loop end in tape_assign_minor?

[Maoyi Xie]

Hi all,

While reading drivers/s390/char/tape_core.c I noticed
something that looks like a past the end iterator pattern.
I would appreciate it if you could take a look and let me
know whether this is a real issue, and whether it is worth
fixing.

The site is tape_assign_minor() (linux-7.1-rc1, around
line 339):

    list_for_each_entry(tmp, &tape_device_list, node) {
            if (minor < tmp->first_minor)
                    break;
            minor += TAPE_MINORS_PER_DEV;
    }
    if (minor >= 256) {
            write_unlock(&tape_device_lock);
            return -ENODEV;
    }
    device->first_minor = minor;
    list_add_tail(&device->node, &tmp->node);

When the loop walks all entries without break (every
existing entry has first_minor at or below the candidate
minor), tmp is past the end. tmp->node then aliases
tape_device_list (the list head) via container_of offset
cancellation. list_add_tail(&device->node, &tape_device_list)
inserts the new device at the tail of the list. That is the
intended behaviour for a sorted insertion where the new
device has the largest first_minor. The dereference of the
past the end iterator is undefined per C11.

Jakob Koschel cleaned up many such sites in 2022, for
example commits 99d8ae4ec8a (tracing: Remove usage of list
iterator variable after the loop), 2966a9918df (clockevents:
Use dedicated list iterator variable) and dc1acd5c946 (dlm:
replace usage of found with dedicated list iterator
variable). This site was not covered.

A candidate fix would track an explicit insertion target.
Declare `struct list_head *insert_before = &tape_device_list`
before the loop. Overwrite it to `&tmp->node` only when the
loop breaks early. The final list_add_tail then reads
`insert_before`. On break that points to the entry right
after the insertion position. On fall-through it stays at
the list head, so list_add_tail appends at the tail. The
behaviour is unchanged in all cases, including an empty
list and a list with one entry.

If this is intentional or already known, please disregard.
Otherwise, I am happy to send a [PATCH] or to leave the fix
to you. Thank you for your time, and sorry for the noise if
this is not actually worth fixing or has already been
spotted.

Thanks,
Maoyi Xie
https://maoyixie.com/

Re: s390/tape: iterator after loop end in tape_assign_minor?

[Heiko Carstens]

On Tue, May 19, 2026 at 06:00:26PM +0800, Maoyi Xie wrote:
> Hi all,
> 
> While reading drivers/s390/char/tape_core.c I noticed
> something that looks like a past the end iterator pattern.
> I would appreciate it if you could take a look and let me
> know whether this is a real issue, and whether it is worth
> fixing.

Thanks for reporting. This something that Jan should look into.
Full quote below.
 
> The site is tape_assign_minor() (linux-7.1-rc1, around
> line 339):
> 
>     list_for_each_entry(tmp, &tape_device_list, node) {
>             if (minor < tmp->first_minor)
>                     break;
>             minor += TAPE_MINORS_PER_DEV;
>     }
>     if (minor >= 256) {
>             write_unlock(&tape_device_lock);
>             return -ENODEV;
>     }
>     device->first_minor = minor;
>     list_add_tail(&device->node, &tmp->node);
> 
> When the loop walks all entries without break (every
> existing entry has first_minor at or below the candidate
> minor), tmp is past the end. tmp->node then aliases
> tape_device_list (the list head) via container_of offset
> cancellation. list_add_tail(&device->node, &tape_device_list)
> inserts the new device at the tail of the list. That is the
> intended behaviour for a sorted insertion where the new
> device has the largest first_minor. The dereference of the
> past the end iterator is undefined per C11.
> 
> Jakob Koschel cleaned up many such sites in 2022, for
> example commits 99d8ae4ec8a (tracing: Remove usage of list
> iterator variable after the loop), 2966a9918df (clockevents:
> Use dedicated list iterator variable) and dc1acd5c946 (dlm:
> replace usage of found with dedicated list iterator
> variable). This site was not covered.
> 
> A candidate fix would track an explicit insertion target.
> Declare `struct list_head *insert_before = &tape_device_list`
> before the loop. Overwrite it to `&tmp->node` only when the
> loop breaks early. The final list_add_tail then reads
> `insert_before`. On break that points to the entry right
> after the insertion position. On fall-through it stays at
> the list head, so list_add_tail appends at the tail. The
> behaviour is unchanged in all cases, including an empty
> list and a list with one entry.
> 
> If this is intentional or already known, please disregard.
> Otherwise, I am happy to send a [PATCH] or to leave the fix
> to you. Thank you for your time, and sorry for the noise if
> this is not actually worth fixing or has already been
> spotted.
> 
> Thanks,
> Maoyi Xie
> https://maoyixie.com/
> 

[PATCH] s390/tape: avoid past-the-end iterator in tape_assign_minor()

[Maoyi Xie]

tape_assign_minor() walks tape_device_list to find the sorted
insertion point, then does list_add_tail(&device->node, &tmp->node).
When the loop runs to the end without break, tmp is past the end and
&tmp->node aliases the list head via container_of. list_add_tail then
appends at the tail, which is the intended result, but the iterator is
dereferenced past the end, which is undefined per the C standard.

Track the insertion point explicitly. insert_before starts at the list
head and is set to &tmp->node only when the loop breaks early. The
list_add_tail uses insert_before, so the behaviour is unchanged in
every case including an empty list.

Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
---
 drivers/s390/char/tape_core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/char/tape_core.c b/drivers/s390/char/tape_core.c
index bd8e3deb1199..361184c05940 100644
--- a/drivers/s390/char/tape_core.c
+++ b/drivers/s390/char/tape_core.c
@@ -330,14 +330,17 @@ __tape_cancel_io(struct tape_device *device, struct tape_request *request)
 static int
 tape_assign_minor(struct tape_device *device)
 {
+	struct list_head *insert_before = &tape_device_list;
 	struct tape_device *tmp;
 	int minor;
 
 	minor = 0;
 	write_lock(&tape_device_lock);
 	list_for_each_entry(tmp, &tape_device_list, node) {
-		if (minor < tmp->first_minor)
+		if (minor < tmp->first_minor) {
+			insert_before = &tmp->node;
 			break;
+		}
 		minor += TAPE_MINORS_PER_DEV;
 	}
 	if (minor >= 256) {
@@ -345,7 +348,7 @@ tape_assign_minor(struct tape_device *device)
 		return -ENODEV;
 	}
 	device->first_minor = minor;
-	list_add_tail(&device->node, &tmp->node);
+	list_add_tail(&device->node, insert_before);
 	write_unlock(&tape_device_lock);
 	return 0;
 }
-- 
2.34.1