From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5B52BC433E0 for ; Thu, 18 Mar 2021 10:08:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2A50E64E86 for ; Thu, 18 Mar 2021 10:08:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229970AbhCRKIK (ORCPT ); Thu, 18 Mar 2021 06:08:10 -0400 Received: from mx2.suse.de ([195.135.220.15]:55654 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229841AbhCRKHr (ORCPT ); Thu, 18 Mar 2021 06:07:47 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 8D79EAD20; Thu, 18 Mar 2021 10:07:46 +0000 (UTC) Date: Thu, 18 Mar 2021 11:07:46 +0100 Message-ID: From: Takashi Iwai To: Johannes Berg Cc: Emmanuel Grumbach , linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: systemd-rfkill regression on 5.11 and later kernels In-Reply-To: <54859a03b8789a2800596067e06c8adb49a107f5.camel@sipsolutions.net> References: <54859a03b8789a2800596067e06c8adb49a107f5.camel@sipsolutions.net> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 18 Mar 2021 10:36:19 +0100, Johannes Berg wrote: > > Hi Takashi, > > Oh yay :-( > > > we've received a bug report about rfkill change that was introduced in > > 5.11. While the systemd-rfkill expects the same size of both read and > > write, the kernel rfkill write cuts off to the old 8 bytes while read > > gives 9 bytes, hence it leads the error: > >   https://github.com/systemd/systemd/issues/18677 > >   https://bugzilla.opensuse.org/show_bug.cgi?id=1183147 > > > As far as I understand from the log in the commit 14486c82612a, this > > sounds like the intended behavior. > > Not really? I don't even understand why we get this behaviour. > > The code is this: > > rfkill_fop_write(): > > ... > /* we don't need the 'hard' variable but accept it */ > if (count < RFKILL_EVENT_SIZE_V1 - 1) > return -EINVAL; > > # this is actually 7 - RFKILL_EVENT_SIZE_V1 is 8 > # (and obviously we get past this if and don't get -EINVAL > > /* > * Copy as much data as we can accept into our 'ev' buffer, > * but tell userspace how much we've copied so it can determine > * our API version even in a write() call, if it cares. > */ > count = min(count, sizeof(ev)); > > # sizeof(ev) should be 9 since 'ev' is the new struct > > if (copy_from_user(&ev, buf, count)) > return -EFAULT; > > ... > ret = 0; > ... > return ret ?: count; > > > > > Ah, no, I see. The bug says: > > EDIT: above is with kernel-core-5.10.16-200.fc33.x86_64. > > So you've recompiled systemd with 5.11 headers, but are running against > 5.10 now, where the short write really was intentional - it lets you > detect that the new fields weren't handled by the kernel. If > > > The other issue is basically this (pre-fix) systemd code: > > l = read(c.rfkill_fd, &event, sizeof(event)); > ... > if (l != RFKILL_EVENT_SIZE_V1) /* log/return error */ > > > > So ... honestly, I don't have all that much sympathy, when the uapi > header explicitly says we want to be able to change the size. But I > guess "no regressions" rules are hard, so ... dunno. I guess we can add > a version/size ioctl and keep using 8 bytes unless you send that? OK, I took a deeper look again, and actually there are two issues in systemd-rfkill code: * It expects 8 bytes returned from read while it reads a struct rfkill_event record. If the code is rebuilt with the latest kernel headers, it breaks due to the change of rfkill_event. That's the error openSUSE bug report points to. * When systemd-rfkill is built with the latest kernel headers but runs on the old kernel code, the write size check fails as you mentioned in the above. That's another part of the github issue. So, with a kernel devs hat on, I share your feeling, that's an application bug. OTOH, the extension of the rfkill_event is, well, not really safe as expected. IMO, if systemd-rfkill is the only one that hits such a problem, we may let the systemd code fixed, as it's obviously buggy. But who knows... Is the extension of rfkill_event mandatory? Can the new entry provided in a different way such as another sysfs record? IOW, if we revert the change, would it break anything else new? thanks, Takashi