SM bus ioctls incorrect in FreeBSD 11

[Andriy Gapon]

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

[Michael Gmelin]

On Fri, 14 Oct 2016 17:50:40 +0300

Post by Andriy Gapon

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.
- 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

For some history on these changes, please see also [1] and [2] (there
were a few discussions and the revision was bumped, I also tried to
get some attention, but not enough it seems).

Given your recent changes to iicbus in HEAD, I think it would be best to
MFC those and go with Option 4 or, if that's to drastic, go with
Option 1. Thanks for cleaning after me.

- Michael

[1]https://lists.freebsd.org/pipermail/freebsd-arch/2015-March/016972.html
[2]https://lists.freebsd.org/pipermail/freebsd-arch/2015-May/017157.html

[Andriy Gapon]

Post by Michael Gmelin
For some history on these changes, please see also [1] and [2] (there
were a few discussions and the revision was bumped, I also tried to
get some attention, but not enough it seems).
Given your recent changes to iicbus in HEAD, I think it would be best to
MFC those and go with Option 4 or, if that's to drastic, go with
Option 1.

I am leaning towards this approach as well.

Post by Michael Gmelin
Thanks for cleaning after me.

You asked for a discussion and reviews.
I can not recall what I was doing at that time, but I completely ignored the
development and for that I can only blame myself.

Post by Michael Gmelin
[1]https://lists.freebsd.org/pipermail/freebsd-arch/2015-March/016972.html
[2]https://lists.freebsd.org/pipermail/freebsd-arch/2015-May/017157.html

I also agree that having a thin library on top of the ioctl would be a convenience.

[Lewis Donzis]

Post by Andriy Gapon
Could you please open a bugzilla issue for the bug?
https://bugs.freebsd.org/bugzilla/

Sure, will do. I was unsure of the effectiveness of that, because I filed a much more serious report about a different issue a couple of weeks ago and have seen no activity or even acknowledgement.

Post by Andriy Gapon
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 was also surprised, but it wasn’t really a big deal, and the new structure might be slightly easier to use. There have been other similar things in 11.0, like __mq_oshandle() disappearing, and more .so files needing to be added to our embedded system, so all things considered, it’s been reasonably smooth moving from 10.3 to 11.0.

Post by Andriy Gapon
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.

Our opinion doesn’t count for much, but I like 2 or 4. Option 1 would essentially obviate the entire purpose of changing the structure. Option 2 basically finishes the job and makes it work properly. Option 3 is, as you say, unappealing. I have no problem with Option 4, obviously we can change our code back to the old way, but assuming there was a good reason for this change in the first place, Option 2 seems more logical.

But whatever y’all decide is fine with us, we’ll just change code to match at the appropriate time.

Thanks,
lew

[Andriy Gapon]

Post by Lewis Donzis
Our opinion doesn’t count for much, but I like 2 or 4. Option 1 would
essentially obviate the entire purpose of changing the structure. Option 2
basically finishes the job and makes it work properly. Option 3 is, as you
say, unappealing. I have no problem with Option 4, obviously we can change
our code back to the old way, but assuming there was a good reason for this
change in the first place, Option 2 seems more logical.
But whatever y’all decide is fine with us, we’ll just change code to match at
the appropriate time.

Anyone interested in the issue, could you please take a look at this review?
https://reviews.freebsd.org/D8430

Thank you.

[Adrian Chadd]

+10 on option 2. Hate to say it, but I'd rather this little corner of
freebsd be "right" before it gets more heavily used.


-a

Post by Andriy Gapon

Post by Lewis Donzis
Our opinion doesn’t count for much, but I like 2 or 4. Option 1 would
essentially obviate the entire purpose of changing the structure. Option 2
basically finishes the job and makes it work properly. Option 3 is, as you
say, unappealing. I have no problem with Option 4, obviously we can change
our code back to the old way, but assuming there was a good reason for this
change in the first place, Option 2 seems more logical.
But whatever y’all decide is fine with us, we’ll just change code to match at
the appropriate time.

Anyone interested in the issue, could you please take a look at this review?
https://reviews.freebsd.org/D8430
Thank you.
--
Andriy Gapon
_______________________________________________
https://lists.freebsd.org/mailman/listinfo/freebsd-arch

[John Baldwin]

Post by Andriy Gapon

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.
- 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.