Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Emmanuel Grumbach <emmanuel.grumbach@intel.com>,
        Johannes Berg <johannes.berg@intel.com>
From:   Hans de Goede <hdegoede@redhat.com>
Cc:     linux-wireless <linux-wireless@vger.kernel.org>
Subject: "rfkill: add a reason to the HW rfkill state" breaks userspace
Message-ID: <efafa85c-c021-14ff-619c-fdd0db53ddbb@redhat.com>
Date:   Tue, 13 Apr 2021 22:31:08 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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/?id=14486c82612a177cb910980c70ba900827ca0894
breaking userspace.

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