| Message ID | 20210503131210.90066-1-benjamin@sipsolutions.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rfkill: Fix reading from rfkill socket | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=476383 ---Test result--- Test Summary: CheckPatch PASS 0.56 seconds GitLint PASS 0.12 seconds Prep - Setup ELL PASS 45.13 seconds Build - Prep PASS 0.15 seconds Build - Configure PASS 7.93 seconds Build - Make PASS 191.25 seconds Make Check PASS 8.55 seconds Make Dist PASS 11.38 seconds Make Dist - Configure PASS 4.79 seconds Make Dist - Make PASS 77.65 seconds Build w/ext ELL - Configure PASS 7.91 seconds Build w/ext ELL - Make PASS 179.36 seconds Details ############################## Test: CheckPatch - PASS Desc: Run checkpatch.pl script with rule in .checkpatch.conf ############################## Test: GitLint - PASS Desc: Run gitlint with rule in .gitlint ############################## Test: Prep - Setup ELL - PASS Desc: Clone, build, and install ELL ############################## Test: Build - Prep - PASS Desc: Prepare environment for build ############################## Test: Build - Configure - PASS Desc: Configure the BlueZ source tree ############################## Test: Build - Make - PASS Desc: Build the BlueZ source tree ############################## Test: Make Check - PASS Desc: Run 'make check' ############################## Test: Make Dist - PASS Desc: Run 'make dist' and build the distribution tarball ############################## Test: Make Dist - Configure - PASS Desc: Configure the source from distribution tarball ############################## Test: Make Dist - Make - PASS Desc: Build the source from distribution tarball ############################## Test: Build w/ext ELL - Configure - PASS Desc: Configure BlueZ source with '--enable-external-ell' configuration ############################## Test: Build w/ext ELL - Make - PASS Desc: Build BlueZ source with '--enable-external-ell' configuration --- Regards, Linux Bluetooth
On Mon, 2021-05-03 at 15:12 +0200, Benjamin Berg wrote: > From: Benjamin Berg <bberg@redhat.com> > > The kernel will always send exactly one event, but the size of the > passed struct will depend on the length of the submitted read() and the > kernel version. i.e. the interface can be extended and we need to > expect > for a read to be longer than expected if we ask for it. > > Fix this by only requesting the needed length and explicitly check the > length against the V1 version of the structure to make the code a bit > more future proof in case the internal copy of the struct is updated to > contain new fields. This fixes a bug in GNOME where to enable Bluetooth, we removed a soft rfkill block on the Bluetooth interface. Without this, the bluetooth rfkill gets unblocked, but bluetoothd doesn't see it as unblocked so never powers it on, causing the UI to appear broken, as we expect Bluetooth devices to be either blocked through rfkill, or powered on. The equivalent gnome-settings-daemon fix (which deals with rfkill) was reviewed by Hans de Goede: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/merge_requests/234 Benjamin, it might be worth resending this with a better commit message explaining exactly what it fixes and referencing the gnome-bluetooth bug: https://gitlab.gnome.org/GNOME/gnome-bluetooth/-/issues/38 Cheers > --- > src/rfkill.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/src/rfkill.c b/src/rfkill.c > index ec9fcdfdd..2099c5ac5 100644 > --- a/src/rfkill.c > +++ b/src/rfkill.c > @@ -53,12 +53,12 @@ struct rfkill_event { > uint8_t soft; > uint8_t hard; > }; > +#define RFKILL_EVENT_SIZE_V1 8 > > static gboolean rfkill_event(GIOChannel *chan, > GIOCondition cond, gpointer data) > { > - unsigned char buf[32]; > - struct rfkill_event *event = (void *) buf; > + struct rfkill_event event = { 0 }; > struct btd_adapter *adapter; > char sysname[PATH_MAX]; > ssize_t len; > @@ -69,34 +69,32 @@ static gboolean rfkill_event(GIOChannel *chan, > > fd = g_io_channel_unix_get_fd(chan); > > - memset(buf, 0, sizeof(buf)); > - > - len = read(fd, buf, sizeof(buf)); > + len = read(fd, &event, sizeof(event)); > if (len < 0) { > if (errno == EAGAIN) > return TRUE; > return FALSE; > } > > - if (len != sizeof(struct rfkill_event)) > + if (len < RFKILL_EVENT_SIZE_V1) > return TRUE; > > DBG("RFKILL event idx %u type %u op %u soft %u hard %u", > - event->idx, event->type, event- > >op, > - event->soft, event- > >hard); > + event.idx, event.type, > event.op, > + event.soft, > event.hard); > > - if (event->soft || event->hard) > + if (event.soft || event.hard) > return TRUE; > > - if (event->op != RFKILL_OP_CHANGE) > + if (event.op != RFKILL_OP_CHANGE) > return TRUE; > > - if (event->type != RFKILL_TYPE_BLUETOOTH && > - event->type != RFKILL_TYPE_ALL) > + if (event.type != RFKILL_TYPE_BLUETOOTH && > + event.type != RFKILL_TYPE_ALL) > return TRUE; > > snprintf(sysname, sizeof(sysname) - 1, > - "/sys/class/rfkill/rfkill%u/name", event->idx); > + "/sys/class/rfkill/rfkill%u/name", event.idx); > > fd = open(sysname, O_RDONLY); > if (fd < 0)
On Thu, 2021-06-10 at 14:45 +0200, Bastien Nocera wrote: > On Mon, 2021-05-03 at 15:12 +0200, Benjamin Berg wrote: > > From: Benjamin Berg <bberg@redhat.com> > > > > The kernel will always send exactly one event, but the size of the > > passed struct will depend on the length of the submitted read() and > > the > > kernel version. i.e. the interface can be extended and we need to > > expect > > for a read to be longer than expected if we ask for it. > > > > Fix this by only requesting the needed length and explicitly check > > the > > length against the V1 version of the structure to make the code a bit > > more future proof in case the internal copy of the struct is updated > > to > > contain new fields. > > This fixes a bug in GNOME where to enable Bluetooth, we removed a soft > rfkill block on the Bluetooth interface. > > Without this, the bluetooth rfkill gets unblocked, but bluetoothd > doesn't see it as unblocked so never powers it on, causing the UI to > appear broken, as we expect Bluetooth devices to be either blocked > through rfkill, or powered on. > > The equivalent gnome-settings-daemon fix (which deals with rfkill) was > reviewed by Hans de Goede: > https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/merge_requests/234 > > Benjamin, it might be worth resending this with a better commit message > explaining exactly what it fixes and referencing the gnome-bluetooth > bug: > https://gitlab.gnome.org/GNOME/gnome-bluetooth/-/issues/38 > It's also been pushed to Fedora rawhide and Fedora 34: https://bodhi.fedoraproject.org/updates/FEDORA-2021-2cd83da751
Hi, On Thu, Jun 10, 2021 at 5:53 AM Bastien Nocera <hadess@hadess.net> wrote: > > On Thu, 2021-06-10 at 14:45 +0200, Bastien Nocera wrote: > > On Mon, 2021-05-03 at 15:12 +0200, Benjamin Berg wrote: > > > From: Benjamin Berg <bberg@redhat.com> > > > > > > The kernel will always send exactly one event, but the size of the > > > passed struct will depend on the length of the submitted read() and > > > the > > > kernel version. i.e. the interface can be extended and we need to > > > expect > > > for a read to be longer than expected if we ask for it. > > > > > > Fix this by only requesting the needed length and explicitly check > > > the > > > length against the V1 version of the structure to make the code a bit > > > more future proof in case the internal copy of the struct is updated > > > to > > > contain new fields. > > > > This fixes a bug in GNOME where to enable Bluetooth, we removed a soft > > rfkill block on the Bluetooth interface. > > > > Without this, the bluetooth rfkill gets unblocked, but bluetoothd > > doesn't see it as unblocked so never powers it on, causing the UI to > > appear broken, as we expect Bluetooth devices to be either blocked > > through rfkill, or powered on. > > > > The equivalent gnome-settings-daemon fix (which deals with rfkill) was > > reviewed by Hans de Goede: > > https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/merge_requests/234 > > > > Benjamin, it might be worth resending this with a better commit message > > explaining exactly what it fixes and referencing the gnome-bluetooth > > bug: > > https://gitlab.gnome.org/GNOME/gnome-bluetooth/-/issues/38 > > > > It's also been pushed to Fedora rawhide and Fedora 34: > https://bodhi.fedoraproject.org/updates/FEDORA-2021-2cd83da751 I missed this one for some reason, it has been applied now.
diff --git a/src/rfkill.c b/src/rfkill.c index ec9fcdfdd..2099c5ac5 100644 --- a/src/rfkill.c +++ b/src/rfkill.c @@ -53,12 +53,12 @@ struct rfkill_event { uint8_t soft; uint8_t hard; }; +#define RFKILL_EVENT_SIZE_V1 8 static gboolean rfkill_event(GIOChannel *chan, GIOCondition cond, gpointer data) { - unsigned char buf[32]; - struct rfkill_event *event = (void *) buf; + struct rfkill_event event = { 0 }; struct btd_adapter *adapter; char sysname[PATH_MAX]; ssize_t len; @@ -69,34 +69,32 @@ static gboolean rfkill_event(GIOChannel *chan, fd = g_io_channel_unix_get_fd(chan); - memset(buf, 0, sizeof(buf)); - - len = read(fd, buf, sizeof(buf)); + len = read(fd, &event, sizeof(event)); if (len < 0) { if (errno == EAGAIN) return TRUE; return FALSE; } - if (len != sizeof(struct rfkill_event)) + if (len < RFKILL_EVENT_SIZE_V1) return TRUE; DBG("RFKILL event idx %u type %u op %u soft %u hard %u", - event->idx, event->type, event->op, - event->soft, event->hard); + event.idx, event.type, event.op, + event.soft, event.hard); - if (event->soft || event->hard) + if (event.soft || event.hard) return TRUE; - if (event->op != RFKILL_OP_CHANGE) + if (event.op != RFKILL_OP_CHANGE) return TRUE; - if (event->type != RFKILL_TYPE_BLUETOOTH && - event->type != RFKILL_TYPE_ALL) + if (event.type != RFKILL_TYPE_BLUETOOTH && + event.type != RFKILL_TYPE_ALL) return TRUE; snprintf(sysname, sizeof(sysname) - 1, - "/sys/class/rfkill/rfkill%u/name", event->idx); + "/sys/class/rfkill/rfkill%u/name", event.idx); fd = open(sysname, O_RDONLY); if (fd < 0)
From: Benjamin Berg <bberg@redhat.com> The kernel will always send exactly one event, but the size of the passed struct will depend on the length of the submitted read() and the kernel version. i.e. the interface can be extended and we need to expect for a read to be longer than expected if we ask for it. Fix this by only requesting the needed length and explicitly check the length against the V1 version of the structure to make the code a bit more future proof in case the internal copy of the struct is updated to contain new fields. --- src/rfkill.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)