Bug 11115

Created attachment 16875 [details]
patch for unaligned memory access in CIFS

I have only tested the patch on AVR32, and it works where it previously would cause an exception and fail.

Created attachment 17003 [details]
More unaligned access fixes in NLS

I've never run across an avr32 machine, but I've seen similar sorts of alignment
warnings pop out of ia64 machines. I think ia64 can cope with them but
it's a performance hit.

Here's what the avr32 manual says:

-----------------[snip]----------------
AVR32 can access data of size byte, halfword, word and doubleword using
dedicated instructions. The memory system can support unaligned
accesses for selected load/store instructions in some implementations.
Any other unaligned access will cause an address exception.
-----------------[snip]----------------

I assume that we're tripping these address exceptions somehow here?

I'm not thrilled with the approach in the patches. The double copying
seems icky. It seems like there ought to be better ways to do this that
wouldn't penalize arches that handle these unaligned accesses w/o issue,
but I don't know how right offhand.

Would like to see how this is handled in other components too ...

John, I've had someone open a RH bug for a similar problem and am interested to see if we can also bring this one to resolution. Do you happen to have a more current patchset for this?

Also, is there a reason you decided to just use memcpy to copy the data to temporary variables instead of just using the get_unaligned/put_unaligned macros?

*** Bug 14569 has been marked as a duplicate of this bug. ***

Created attachment 43692 [details]
patch -- fix up some unaligned accesses in cifs

This is a preliminary patchset, tested on x86_64 and doesn't seem to break anything...

This first stops the current practice of "converting" the bytecount in a received packet to host-endian format. It then turns the BCC and BCC_LE macros into inlined functions that use get/put_unaligned_le16 to fetch and store the it. Several other places in validate_t2 pointed out by John are also converted to use get_unaligned_le16.

For the unicode functions, I've converted them to pass a __u16 stack variable to
char2uni. That's then copied to the destination buffer using put_unaligned_le16. With that change, we shouldn't need to change any NLS code.

I don't have easy access to arches that have the problems that this is intended to fix. If someone here could help test and verify this patch that would be a great help.

Also note that this is probably not a comprehensive fix for CIFS. There are a number places in the code that simply seem to do a "le16_to_cpu" or "cpu_to_le16". It's possible some of those may also need fixing, but we'll need to look at them on a case-by-case basis.

Actually, this patch will probably break big-endian arches. There are a number of places in the cifs code that access the ByteCount directly and hence rely on it to be host-endian.

I'll probably have to step back and take another approach to this...

Ok, I've respun the patchset and posted it upstream. Comments and testing would be appreciated:

    http://thread.gmane.org/gmane.linux.kernel.cifs/2308

...I don't think this will be a complete solution for CIFS on avr32 however. There are a number of places that access fields directly in a received buffer or do the same in a buffer that it's going to emit. Those will likely need to be fixed in a second pass.

I think this is mostly fixed now upstream. There may be some other places that need work here, but no one has reported them. Please reopen this if you see any more problems of this sort in current mainline kernels.