From owner-freebsd-current@freebsd.org Fri Oct 14 18:35:29 2016 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 09BE9C122F9; Fri, 14 Oct 2016 18:35:29 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from mail.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id D697226B; Fri, 14 Oct 2016 18:35:28 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id 8934410AF90; Fri, 14 Oct 2016 14:35:27 -0400 (EDT) From: John Baldwin To: Andriy Gapon Cc: Lewis Donzis , freebsd-current@freebsd.org, "freebsd-arch@freebsd.org" Subject: Re: SM bus ioctls incorrect in FreeBSD 11 Date: Fri, 14 Oct 2016 11:34:45 -0700 Message-ID: <13130323.4rNJusEcix@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.0-PRERELEASE; KDE/4.14.10; amd64; ; ) In-Reply-To: References: <06929AC5-D350-4236-A813-56C862B58174@perftech.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (mail.baldwin.cx); Fri, 14 Oct 2016 14:35:27 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.99.2 at mail.baldwin.cx X-Virus-Status: Clean X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Oct 2016 18:35:29 -0000 On Friday, October 14, 2016 05:50:40 PM Andriy Gapon wrote: > On 14/10/2016 00:39, Lewis Donzis wrote: > > After upgrading to FreeBSD 11.0 and changing source code to use the= new > > version of =E2=80=9Cstruct smbcmd=E2=80=9D, some commands are not w= orking as documented, > > specifically those that read data. > >=20 > > As an example, SMB_READW is documented as returning the word read f= rom the > > device in rdata.word. However, this doesn=E2=80=99t happen, I thin= k because the > > ioctl request value is defined using _IOW(), so the kernel doesn=E2= =80=99t copy the > > data it read back out. > >=20 > > In prior versions, the structure had only a pointer to the data, an= d the > > smb.c code used copyout() to transfer the data back to userland. > >=20 > > As a temporary work-around, we added code to set rbuf to point to r= data.word > > and rcount to two. >=20 > Lewis, >=20 > thank you for the report. This is a bug indeed and your analysis is = correct. > Could you please open a bugzilla issue for the bug? > https://bugs.freebsd.org/bugzilla/ >=20 > Looking at ports commit 385155 > https://svnweb.freebsd.org/ports/head/sysutils/xmbmon/files/patch-get= MB-smb_ioctl.c?r1=3D385155&r2=3D385154&pathrev=3D385155 > I see that it also used the approach that you use as a workaround. > And that port commit is by Michael Gmelin who made the change to smb.= h in > r281985 https://svnweb.freebsd.org/base?view=3Drevision&revision=3D28= 1985 > So, I am not sure if the documented approach was known to not work. >=20 > The src change is described as "Expand SMBUS API ...", but in fact it= also > _changed_ the existing ioctls. And both binary compatibility and pro= gramming > compatibility were broken because of how struct smbcmd was changed. > In FreeBSD we try to not do that without a very strong reason, but al= as. > And, as you report, the change was not done entirely correctly. >=20 > I see several possibilities now. >=20 > Option 1. Change the documentation to reflect the actual behavior. > In this case data.rdata will remain unusable and unused. No interfac= e changes. >=20 > Option 2. Redefine SMB_READB, SMB_READW and SMB_PCALL ioctls using _I= OWR, so > that data.rdata could be returned from kernel. This seems like a pro= per fix, > but it is another binary level incompatibility. >=20 > Option 3. Use a horrible hack to discover a userland address of smbc= md and > explicitly copyout to data.rdata. No interface incompatibilities, bu= t it will > be a horrible hack. Besides, not sure how feasible it is. >=20 > Option 4. Revert smb ioctl changes to what they used to be before r2= 81985. > Personally, I would prefer this approach. But now that the new inter= face is in > 11.0, it means another interface change just like Option 2. >=20 > I would like to hear other developers' opinions about this situation.= >=20 > P.S. > Two changes that I want to do no matter which course of action we sel= ect are: > - revert SMB_MAXBLOCKSIZE to 32 > - remove SMB_TRANS as it does not map to anything defined by the SMBu= s > specification and it can not be implemented for most, if not all, > SMBus controllers During the review the assumption was that breaking the ABI wasn't great= , but that we could always fix it by adding compat handlers for the old ioctl valu= es. If those are needed then they need to be restored. If the new API is usef= ul then it needs to be fixed which I think is option 2. I think it is new enough = to just fix without support compat shims for the broken version of it. Unfortunately I don't know SMBus well enough to comment on your latter = changes, so I will defer to your call on those. --=20 John Baldwin