Some patches require more comments than can reasonably be put in the patch table, or invite questions that may as well be answered here rather than generating email.
If this field reads Yes, then it indicates that it appears, from inspection, that there remains some locking issues just in the way that open and release functions interact with each other. Details on this can be found by clicking on the Yes field in the table.
If this field reads Yes, then it indicates that it appears, from inspection, that there remains some locking issues elsewhere in the driver file. Since it was not a goal to detect and find these, they were usually found incidentally while looking at BKL usage in open and release functions. For this reason, most entries are ?, indicating no inspection was done and no conclusions can be drawn. When the field reads Yes, details can be found by clicking on the Yes field in the table, but again there is no guarantee they are the only SMP issues in the driver.
If the field No is found here, it is a positive indication that the entire driver has been inspected and there does not appear to be any SMP issues.The private_data field of the struct file is modified without safeguards in mtrr_file_add(). Since a file descriptor may be inherited across forks (clones), this "private data" may actually be modified by two or more processes, and should utilize locking to be safe. Currently, ioctl's are serialized across the system by the BKL in sys_ioctl(), so this is not broken right now, but it is reasonable to presume that the BKL's use in sys_ioctl() may be deprecated in the future as the BKL is slowly removed from the kernel code. At the next convenient opportunity, it might be advisable for this module to code up its own locking for this section of code.
The ready field of the struct joystick_status is modified without safeguards in atari_joystick_interrupt(). The same variable may be set to zero in both release_joystick() and in open_joystick(). On a monoprocessor machine, those things cannot happen while in an interrupt, but on a multi-processor machine, they may. It remains to be seen if this code is ever used on a multiprocessor machine, of course. Even if it is not, however, to prevent cut 'n' paste artists from using this as a poor template, a comment should at least be inserted to note this.
initrd_start is set and used in many places without the benefit of a lock. It's not clear if this is safe because of some implicit single threading or, in fact, not safe at all.
Not an error, but a note: dtlk_busy is set to 0 but never is set to 1. It is unnecessary now.
The use of cli/save_flags can be horrible on SMP machines.
pp->pdev() is set via an ioctl() with no locking in place. Currently, all ioctl's are synchronized through the BKL in sys_ioctl(), but it would be prudent to presume that use will be deprecated in the future. Since a file descriptor may be dup'd explicitly through dup() or dup2() or implicitly by a fork/clone, we can't be sure we're the only one operating on this data. Locking should be present to cover this admittedly small window. Similar windows appear to exist in pp_read() and pp_write(), using the same logic about multiple processors, and there isn't a synchronization mechanism in place there.
aux_count is unguarded, and a simple increment/decrement operator is not atomic and can't guarantee that two processes won't make it through here.
In the open/release functions, the variable wdt_is_open remains unguarded. There are other unguarded variables as well, and correcting the wdt_is_open problem so that you are limited to a single open doesn't help you. A file descriptor can be in use more than once through clone/fork, so the code in fop_write() cannot presume that it is safe to modify global variables like wdt_expect_close.
In the open/release functions, the variable timer_alive remains unguarded, and thus leaves it possible for two processes to believe they have exclusive use of the device.
In the open function, a comment suggests that file_count(filp) is a good indication that we are the only person opening this device. So far as I know, this is not true (multiple opens will use unique file *'s), and many other assumptions then fail if we do not have an exclusive open.
However, beyond that, it is possible through fork/clone to share an opened file descriptor, and so functions such as qic02_tape_write() can not assume they are the only processes accessing or modifying global variables such as doing_read.
In the open/release functions, the variable wdt_is_open remains unguarded. There are other unguarded variables as well, and correcting the wdt_is_open problem so that you are limited to a single open doesn't help you. A file descriptor can be in use more than once through clone/fork, so the code in fop_write() cannot presume that it is safe to modify global variables like wdt_expect_close.
In the open/release functions, the variable timer_alive remains unguarded, and thus leaves it possible for two processes to believe they have exclusive use of the device.
In the open/release functions, the variable timer_alive remains unguarded, and thus leaves it possible for two processes to believe they have exclusive use of the device. I would guess, additionally, that the outb() require that exclusivity.
The variable list is dynamically allocated at open time -- all well and good. But one of its fields points to a statically allocated (and global) structure, evdev_table. Since this is not an exclusive open device, it means that opens and closes that happen on the same device could trip over each other and destroy the integrity of the linked list represented by evdev_table.
The variable list is dynamically allocated at open time -- all well and good. But one of its fields points to a statically allocated (and global) structure, joydev_table. Since this is not an exclusive open device, it means that opens and closes that happen on the same device could trip over each other and destroy the integrity of the linked list represented by joydev_table.
The variable list is dynamically allocated at open time -- all well and good. But one of its fields points to a statically allocated (and global) structure, mousedev_table. Since this is not an exclusive open device, it means that opens and closes that happen on the same device could trip over each other and destroy the integrity of the linked list represented by mousedev_table. In addition, such attributes as open are also subject to the same problems. A lock should be placed on mousedev_table.
The global variable capidev_openlist is traversed as part of capidev_alloc(). It should be guarded against changes during that traversal. This is true of all places in the code where capidev_openlist is traversed and/or modified. All the global variables declared near this variable are probably subject to similar caveats.
The private_data field of the file structure is used to keep a pointer into a struct divert_info. This is a linked list formed (in part) by global structures. This is one of the few cases where lock_kernel() was uniformly (and correctly) applied in both open and close, and the patch now replaces that with another lock. However, neither the old code nor the new protects this list when we modify it (and the usage_cnt) in isdn_divert_read().
Since ds_open() does nothing to guarantee exclusive opens, the variables s->state and s->user are in danger of being corrupted by multiple simultaneous opens. Similar areas of possible corruption exist in ds_release().
There are several places in the open routine alone where there are SMP issues. sdp->detached, for example, is referenced in sg_open even while it is possible it might be being changed in sg_detach(). access_count is incremented without the benefit of locking, causing problems in the case of both parallel opens and closes. In general, while some portions of the driver take care to utilize locks, there are still many areas that do not.
It's not clear that newport_wait() in the open routine actually works. It appears that i starts at 0 and is never incremented. If newport_wait() does not work to create an exclusive open, then the open is not an exclusive open and there are operations in sgi_graphics_open() which are not protected.
The variable mouse_opened is not properly guarded against simultaneous changes.
The variable hiddev_table is accessed (and modified) in many hiddev routines (including both hiddev_open() and hiddev_release()) without the benefit of any locking.
The variable open_map is accessed and modified in the open routine without the benefit of a lock. This means there is no guaranteed exclusive open, so modification to netlink_user[minor] later in the open routine is dangerous.
Return to global lock patches page.
Return to LSE home page.
Last updated: 11/21/01