> > Hi, > > I've been debugging a userspace rfkill issue today which boils down to the > "rfkill: add a reason to the HW rfkill state" patch: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i > d=14486c82612a177cb910980c70ba900827ca0894 > breaking userspace. This has been rolled back by: https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git/commit/?id=71826654ce40112f0651b6f4e94c422354f4adb6 Other userspace broke (systemd) so Johannes rolled this back by default. Userspace that is interested in the new byte will read 9 bytes. > > It is too late to fix this now since we likely also have new userspace > depending on the new API, but I still thought I should report this. > > I've submitted a fix for the problematic userspace bits here: > https://gitlab.gnome.org/GNOME/gnome-settings-daemon/- > /merge_requests/232 > > Let me quote the commit msg which explains the problem: > > """ > Access to a /dev/foo device should never use buffered mode. > > While debugging a gsd-rfkill issue I noticed in the g_debug output that the > rfkill-glib.c code now seems to be receiving bogus events. > Doing a strace I noticed some read(dev_rfkill_fd, buf, 8) calls, even though > we call: > g_io_channel_read_chars(..., sizeof(struct rfkill_event, ...) Which requests 9 > bytes. > > The problem is the kernel expects us to read 1 event per read() system-call > and it will throw away excess data. The idea is here that the rfkill_event > struct can be extended by adding new fields at the end and then userspace > code compiled against older kernel headers will still work since it will only > read the fields it knows in a single call and the extra fields are thrown away. > > Since the rfkill-glib.c code was using buffered-io and asking > g_io_channel_read_chars for 9 bytes when compiled against recent kernel > headers, what would happen is that 2 events would be consumed in 2 > read(fd, buf, 8) syscalls and then the first byte of the second event read > would be appended to the previous event and the remaining 7 bytes would > be used as the first 7 bytes for the next event (and eventually completed > with the first 2 bytes of the next event, etc.). Leading to completely bogus > events. > > Enabling unbuffered mode fixes this. > > (before the kernel change the rfkill_event struct was 8 bytes large which > allowed us to get away with using buffered io here.) """ > > Note this is new userspace on a new kernel actually being broken. > > I believe that the new userspace (expecting 9 bytes) on old kernel will also > be broken, since a naive userspace implementation will do: > > if (read(fd, buf, sizeof(struct rfkill_event)) != sizeof(struct > rfkill_event)) > /* Do error */ > > Which means that after a recompile on a new kernel it will expect 9 bytes > from a read call an if it gets only 8 then it will consider that an error (or worse > it could try to do a second read to make-up for the missing byte). Note that > gnome-settings-daemon still has the new-userspace on old-kernel issue > even after my fix... > > I believe that all that we can do now is fix userspace where necessary :| but > this is something to keep in mind for future fixes. > > Regards, > > Hans
