Re: [dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks

[Neil Horman]

On Tue, Aug 07, 2018 at 03:14:22PM +0200, Jean Delvare wrote:
> On Tue, 7 Aug 2018 07:33:54 -0400, Neil Horman wrote:
> > On Tue, Aug 07, 2018 at 09:29:44AM +0200, Jean Delvare wrote:
> > > That's tempting, however it does not seem like these macros are dealing
> > > with unaligned access. If we are running on a little-endian machine
> > > which does not support unaligned memory access (hello IA-64!), we end
> > > up with the following definitions:
> > > 
> > > #  define le16toh(x) (x)
> > > #  define le32toh(x) (x)
> > > #  define le64toh(x) (x)
> > > 
> > > which will surely crash.  
> > I don't think so, at least not on linux, where the kernel installs a 
> > handler to
> > fix up unaligned accesses.
> 
> Is it a "recent" addition? I am certain that it used to crash, back in
> 2003 - I did not implement ALIGNMENT_WORKAROUND just for fun. The git
> log also has evidence that gcc used to complain about unaligned
> structure member copies. If you can point me to the kernel piece of
> code which deals with that, I'm interested.
Look to arch/ia64/kernel/unaligned.c

Git history has it as far back as 2005, but thats the import to the current
tree, so I'm guessing it goes back a bit farther than that.

> That being said, I am
> afraid that it must be slower to do things wrong and let the kernel
> cope with the mess, than do the right thing directly?
> 
It is slower of course, because you have to take a trap and fix up the access
(and then the kernel logs it).  Its not something you want to happen, I'm just
saying its no longer fatal.  That said, the right thing to use is the
get_unaligned macro to avoid the misaligned access in the first place.

> I guess I could just connect to an IA-64 system at work and try, but if
> it doesn't work and I crash the machine, my colleagues might
> complain ;-)
> 
> > Either way, it should be pretty easily fixable I
> > think by defining the macros as:
> > 
> > #define WORD(x) le16toh(get_unaligned(x))
> 
> get_unaligned() doesn't seem to be a standard user-space function.
> 
Its standard for linux, but no, its not part of any specification.

> > ...
> > That may not be exactly right (might need to serialize the macro use), but 
> > its
> > close.
> 
> Doesn't sound that much more simple than what I did ;-)
> 
> > Either way, I guess it doesn't really matter, this was more about not
> > maintaining duplicate code for endian macros, but since its already written 
> > and
> > likely won't change again for another decade, theres less value in this.
> 
> Well I'm always interested in ways to clean up my code, so it remains
> an interesting discussion.
> 
> > (...)
> > Sure, as soon as I get it back up and running.  Also note, I'm starting to 
> > think
> > it might have a problem in the offset of its protocol record area (I'll need
> 
> Oh you think? ;-D
> 
> > another system to compare it too, which I'll have later this week I think).
> > Either way, it might be useful to have an erroneous dump for testing 
> > purposes
> 
> Having several samples to test a new features is always good.
> 
> And yes, I have a number of known bad DMI tables in my test suite, to
> exercise the error paths. But I'm not sure that's what you have... See
> my review of your patch.
I keep looking at the protocol count field, which the spec lists as being at
0x6+n bytes, where n is defined as the interface specific data length from
earlier in the structure, but I keep finding it on this system at 0x7+n, so
either they put the count 1 too many bytes in, or the spec if vague as to
weather the interface specific data length includes the length byte itself.

Neil

> 
> -- 
> Jean Delvare
> SUSE L3 Support
>