Re: SM bus ioctls incorrect in FreeBSD 11

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 “struct smbcmd”, some commands are not working as documented,
> > specifically those that read data.
> > 
> > As an example, SMB_READW is documented as returning the word read from the
> > device in rdata.word.  However, this doesn’t happen, I think because the
> > ioctl request value is defined using _IOW(), so the kernel doesn’t copy the
> > data it read back out.
> > 
> > In prior versions, the structure had only a pointer to the data, and the
> > smb.c code used copyout() to transfer the data back to userland.
> > 
> > As a temporary work-around, we added code to set rbuf to point to rdata.word
> > and rcount to two.
> 
> Lewis,
> 
> 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/
> 
> Looking at ports commit 385155
> https://svnweb.freebsd.org/ports/head/sysutils/xmbmon/files/patch-getMB-smb_ioctl.c?r1=385155&r2=385154&pathrev=385155
> 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=revision&revision=281985
> So, I am not sure if the documented approach was known to not work.
> 
> The src change is described as "Expand SMBUS API ...", but in fact it also
> _changed_ the existing ioctls.  And both binary compatibility and programming
> compatibility were broken because of how struct smbcmd was changed.
> In FreeBSD we try to not do that without a very strong reason, but alas.
> And, as you report, the change was not done entirely correctly.
> 
> I see several possibilities now.
> 
> Option 1.  Change the documentation to reflect the actual behavior.
> In this case data.rdata will remain unusable and unused.  No interface changes.
> 
> Option 2. Redefine SMB_READB, SMB_READW and SMB_PCALL ioctls using _IOWR, so
> that data.rdata could be returned from kernel.  This seems like a proper fix,
> but it is another binary level incompatibility.
> 
> Option 3.  Use a horrible hack to discover a userland address of smbcmd and
> explicitly copyout to data.rdata.  No interface incompatibilities, but it will
> be a horrible hack.  Besides, not sure how feasible it is.
> 
> Option 4.  Revert smb ioctl changes to what they used to be before r281985.
> Personally, I would prefer this approach.  But now that the new interface is in
> 11.0, it means another interface change just like Option 2.
> 
> I would like to hear other developers' opinions about this situation.
> 
> P.S.
> Two changes that I want to do no matter which course of action we select are:
> - revert SMB_MAXBLOCKSIZE to 32
> - remove SMB_TRANS as it does not map to anything defined by the SMBus
>   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 values.  If
those are needed then they need to be restored.  If the new API is useful 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.

-- 
John Baldwin