diff mbox

[linux-cifs-client] PATCH: alignment changes for CIFS VFS

Message ID 4D81CA9C484D3842B958A9CD18344D1D04915FAD@c4mail02.control4.com (mailing list archive)
State New, archived
Headers show

Commit Message

Craig Matsuura June 29, 2009, 5:30 p.m. UTC 
No comments regarding this patch?  I sent a more recent patch, but the
moderator has not approve it apparently?  I was hoping to get some
feedback as if anynone else has seen such problems with alignment.  I
wanted to fix this in our kernel so there are not fixups.

 

Thanks,

Craig

 

From: linux-cifs-client-bounces+cmatsuura=control4.com@lists.samba.org
[mailto:linux-cifs-client-bounces+cmatsuura=control4.com@lists.samba.org
] On Behalf Of Craig Matsuura
Sent: Thursday, June 25, 2009 8:19 PM
To: linux-cifs-client@lists.samba.org
Subject: [linux-cifs-client] PATCH: alignment changes for CIFS VFS

 

I saw the following patch at http://patchwork.kernel.org/patch/23910/. I
applied this patch to help me resolve some issues I was having with the
CIFS VFS. So far so good. I have gone as far as to make several more
changes and wanted to contribute back the changes and have some review.



 

I am on a 2.6.28 kernel on a arm9 based system using a gcc-4.3.2 cross
compiler. I have been experiencing alignment traps on my system (I
viewed them from the /proc/cpu/alignment). To track them down I
commented out the alignment handler in the arch/arm/mm/alignment.c. (Did
this to just track down the unalignments)



 

I have made several changes to the CIFS VFS and I have a patch (that
includes the patch above). My patch fixed several unalignments I was
seeing in my kernel.



 

c4davinci-cifs-alignment.patch
return rc;

@@ -260,13 +260,10 @@ static int decode_unicode_ssetup(char **
return rc;

kfree(ses->serverDomain);
- ses->serverDomain = kzalloc(2 * (len + 1), GFP_KERNEL); /* BB FIXME
wrong length */
- if (ses->serverDomain != NULL) {
+ ses->serverDomain = kzalloc((4 * len) + 2, GFP_KERNEL);
+ if (ses->serverDomain != NULL)
cifs_strfromUCS_le(ses->serverDomain, (__le16 *)data, len,
nls_cp);
- ses->serverDomain[2*len] = 0;
- ses->serverDomain[(2*len) + 1] = 0;
- }
data += 2 * (len + 1);
words_left -= len + 1;

@@ -575,7 +572,8 @@ CIFS_SessSetup(unsigned int xid, struct 
count = iov[1].iov_len + iov[2].iov_len;
smb_buf->smb_buf_length += count;

- BCC_LE(smb_buf) = cpu_to_le16(count);
+ /* BCC_LE(smb_buf) = cpu_to_le16(count); */
+ put_unaligned_le16(count,(char *)smb_buf + sizeof(struct smb_hdr) + (2
* smb_buf->WordCount));

rc = SendReceive2(xid, ses, iov, 3 /* num_iovecs */, &resp_buf_type,
CIFS_STD_OP /* not long */ | CIFS_LOG_ERROR);
@@ -600,7 +598,8 @@ CIFS_SessSetup(unsigned int xid, struct 
cFYI(1, ("UID = %d ", ses->Suid));
/* response can have either 3 or 4 word count - Samba sends 3 */
/* and lanman response is 3 */
- bytes_remaining = BCC(smb_buf);
+ /*bytes_remaining = BCC(smb_buf);*/
+ bytes_remaining = get_unaligned((int *) ((char *)smb_buf +
sizeof(struct smb_hdr) + (2 * smb_buf->WordCount)));
bcc_ptr = pByteArea(smb_buf);

if (smb_buf->WordCount == 4) {
@@ -616,12 +615,18 @@ CIFS_SessSetup(unsigned int xid, struct 
}

/* BB check if Unicode and decode strings */
- if (smb_buf->Flags2 & SMBFLG2_UNICODE)
+ if (smb_buf->Flags2 & SMBFLG2_UNICODE) {
+ /* unicode string area must be word-aligned */
+ if (((unsigned long) bcc_ptr - (unsigned long) smb_buf) % 2) {
+ ++bcc_ptr;
+ --bytes_remaining;
+ }
rc = decode_unicode_ssetup(&bcc_ptr, bytes_remaining,
ses, nls_cp);
- else
+ } else {
rc = decode_ascii_ssetup(&bcc_ptr, bytes_remaining,
ses, nls_cp);
+ }

ssetup_exit:
if (spnego_key) {

Comments

Jeff Layton June 30, 2009, 12:17 a.m. UTC | #1 
On Mon, 2009-06-29 at 11:30 -0600, Craig Matsuura wrote:
> No comments regarding this patch?  I sent a more recent patch, but the
> moderator has not approve it apparently?  I was hoping to get some
> feedback as if anynone else has seen such problems with alignment.  I
> wanted to fix this in our kernel so there are not fixups.

A few general comments...

First, the patch was posted as HTML mail which pretty much makes it
impossible for anyone to use. It also has a lot of word-wrap munging
which further causes problems. I generally use git-format-patch and
git-send-email for sending patches. Send them to yourself first until
you get the hang of it and convince yourself that your mailer hasn't
messed it up.

The patch looks like it's based on an older kernel. If you want it in
mainline you'll need to update it to the latest. I recommend pulling
down Steve French's tree and making sure that it applies there since
that represents the bleeding edge CIFS code.

Aside from the word wrap, this kind of stuff needs be removed:

+/*printk("*pBCC=%x, get_unaligned=%x\n",(*(u16 *)pBCC),
get_unaligned((u16 *) 
pBCC));*/

...if you want to add debugging messages, use the cFYI macro.

Also, places where you commented out existing code and replaced it with
new code should just remove the old code.

Comments in the middle of a line like this are really ugly. Yes, CIFS
has tons of them, but let's avoid adding new ones:

+                                           (get_unaligned((u16 *)
((char *)smb_buffer_response + sizeof(struct 
smb_hdr) + (2 *
smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/ - 
1) / 2;

While you're at it, this whole get_unaligned construction really cries
out for a helper function or macro. This stuff was already hard to read
and you're making it even harder. Making it more readable would be a
good thing.
Craig Matsuura June 30, 2009, 12:31 a.m. UTC | #2 
Thanks for the comments.  I consider a macro to replace the ugly, get_unaligned (Good idea).  I was wonder if the change to using a get_unaligned was a 
sound solution to the problem I was encountering.  I'm sure this is not hi priority as this is not x86 specific it is more of a problem on the ARM.  I would only 
want the changes in the mainly if you thought they made sense.  

I can work on cleaning up the debugs printk's (sorry about that).  And I could add a macro.  You are correct about the older kernel, our device is based on a 
2.6.28.10.  I do have a few questions.

I am unfamiliar with cFYI macro and how it is used, I guess I can just google.  Is there a better place?

As for git formatted patches I am not familiar with the git patch creation.  Where is the best place to learn about git patch creation?

It has been a while since I contributed anything to community so I apologize for the html and ugly patch. (Hope I got it fixed this time).

Thanks,
Craig


On Monday 29 June 2009 6:17:42 pm Jeff Layton wrote:
> On Mon, 2009-06-29 at 11:30 -0600, Craig Matsuura wrote:
> > No comments regarding this patch?  I sent a more recent patch, but the
> > moderator has not approve it apparently?  I was hoping to get some
> > feedback as if anynone else has seen such problems with alignment.  I
> > wanted to fix this in our kernel so there are not fixups.
>
> A few general comments...
>
> First, the patch was posted as HTML mail which pretty much makes it
> impossible for anyone to use. It also has a lot of word-wrap munging
> which further causes problems. I generally use git-format-patch and
> git-send-email for sending patches. Send them to yourself first until
> you get the hang of it and convince yourself that your mailer hasn't
> messed it up.
>
> The patch looks like it's based on an older kernel. If you want it in
> mainline you'll need to update it to the latest. I recommend pulling
> down Steve French's tree and making sure that it applies there since
> that represents the bleeding edge CIFS code.
>
> Aside from the word wrap, this kind of stuff needs be removed:
>
> +/*printk("*pBCC=%x, get_unaligned=%x\n",(*(u16 *)pBCC),
> get_unaligned((u16 *)
> pBCC));*/
>
> ...if you want to add debugging messages, use the cFYI macro.
>
> Also, places where you commented out existing code and replaced it with
> new code should just remove the old code.
>
> Comments in the middle of a line like this are really ugly. Yes, CIFS
> has tons of them, but let's avoid adding new ones:
>
> +                                           (get_unaligned((u16 *)
> ((char *)smb_buffer_response + sizeof(struct
> smb_hdr) + (2 *
> smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/ -
> 1) / 2;
>
> While you're at it, this whole get_unaligned construction really cries
> out for a helper function or macro. This stuff was already hard to read
> and you're making it even harder. Making it more readable would be a
> good thing.
Jeff Layton June 30, 2009, 12:42 a.m. UTC | #3 
On Mon, 2009-06-29 at 18:31 -0600, Craig Matsuura wrote:
> Thanks for the comments.  I consider a macro to replace the ugly, get_unaligned (Good idea).  I was wonder if the change to using a get_unaligned was a 
> sound solution to the problem I was encountering.  I'm sure this is not hi priority as this is not x86 specific it is more of a problem on the ARM.  I would only 
> want the changes in the mainly if you thought they made sense.  

Well, Linux is not x86 only (of course). While I don't personally work
much with more obscure arches, I have no problem with patches that make
things work better on those arches. I don't have a good way of testing
them though. You may want to cc lkml or linux-fsdevel when you send to
get feedback from people who do more work on those arches.

As far as if they're needed...I don't know. I assume they are or you
wouldn't have posted a patch, right? Justifying the need for a patch is
good info to put into the patch description when you send it.

> I can work on cleaning up the debugs printk's (sorry about that).  And I could add a macro.  You are correct about the older kernel, our device is based on a 
> 2.6.28.10.  I do have a few questions.
> 
> I am unfamiliar with cFYI macro and how it is used, I guess I can just google.  Is there a better place?
> 

Read the source? There are cFYI macros sprinkled all over the CIFS code.

> As for git formatted patches I am not familiar with the git patch creation.  Where is the best place to learn about git patch creation?
> 

This is a good git starter document:

http://www.kernel.org/pub/software/scm/git/docs/everyday.html

> It has been a while since I contributed anything to community so I apologize for the html and ugly patch. (Hope I got it fixed this time).

No problem. Most of us don't mind helping newbies as long as they're
willing to learn.

Cheers,
diff mbox

Patch

diff -rupw a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
--- a/fs/cifs/cifssmb.c 2009-06-25 16:55:30.000000000 -0600
+++ b/fs/cifs/cifssmb.c 2009-06-25 16:20:45.000000000 -0600
@@ -32,6 +32,7 @@ 
#include <linux/vfs.h>
#include <linux/posix_acl_xattr.h>
#include <asm/uaccess.h>
+#include <asm/unaligned.h>
#include "cifspdu.h"
#include "cifsglob.h"
#include "cifsacl.h"
@@ -431,7 +432,8 @@  static int validate_t2(struct smb_t2_rsp
pBCC = (pSMB->hdr.WordCount * 2) +
sizeof(struct smb_hdr) +
(char *)pSMB;
- if ((total_size <= (*(u16 *)pBCC)) &&
+/*printk("*pBCC=%x, get_unaligned=%x\n",(*(u16 *)pBCC),
get_unaligned((u16 *) pBCC));*/
+ if ((total_size <= get_unaligned_le16((u16 *)pBCC) /*(*(u16 *)pBCC)*/)
&&
(total_size <
CIFSMaxBufSize+MAX_CIFS_HDR_SIZE)) {
return 0;
diff -rupw a/fs/cifs/connect.c b/fs/cifs/connect.c
--- a/fs/cifs/connect.c 2009-06-25 16:55:30.000000000 -0600
+++ b/fs/cifs/connect.c 2009-06-25 16:32:02.000000000 -0600
@@ -35,6 +35,7 @@ 
#include <linux/freezer.h>
#include <asm/uaccess.h>
#include <asm/processor.h>
+#include <asm/unaligned.h>
#include "cifspdu.h"
#include "cifsglob.h"
#include "cifsproto.h"
@@ -2548,13 +2549,13 @@  CIFSSessSetup(unsigned int xid, struct c
if (smb_buffer->Flags2 & SMBFLG2_UNICODE) {
if ((long) (bcc_ptr) % 2) {
remaining_words =
- (BCC(smb_buffer_response) - 1) / 2;
+ (get_unaligned((u16 *) ((char *)smb_buffer_response + sizeof(struct
smb_hdr) + (2 *
smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/ - 1) / 2;
/* Unicode strings must be word
aligned */
bcc_ptr++;
} else {
remaining_words =
- BCC(smb_buffer_response) / 2;
+ get_unaligned((u16 *) ((char *)smb_buffer_response + sizeof(struct
smb_hdr) + (2 *
smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/ / 2;
}
len =
UniStrnlen((wchar_t *) bcc_ptr,
@@ -2636,7 +2637,7 @@  CIFSSessSetup(unsigned int xid, struct c
len = strnlen(bcc_ptr, 1024);
if (((long) bcc_ptr + len) - (long)
pByteArea(smb_buffer_response)
- <= BCC(smb_buffer_response)) {
+ <= get_unaligned((u16 *) ((char *)smb_buffer_response + sizeof(struct
smb_hdr) + (2 *
smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/) {
kfree(ses->serverOS);
ses->serverOS = kzalloc(len + 1,
GFP_KERNEL);
@@ -2890,7 +2891,7 @@  CIFSNTLMSSPNegotiateSessSetup(unsigned i
if (smb_buffer->Flags2 & SMBFLG2_UNICODE) {
if ((long) (bcc_ptr) % 2) {
remaining_words =
- (BCC(smb_buffer_response)
+ (get_unaligned((u16 *) ((char *)smb_buffer_response + sizeof(struct
smb_hdr) + (2 *
smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/
- 1) / 2;
/* Must word align unicode strings */
bcc_ptr++;
@@ -2977,7 +2978,7 @@  CIFSNTLMSSPNegotiateSessSetup(unsigned i
len = strnlen(bcc_ptr, 1024);
if (((long) bcc_ptr + len) - (long)
pByteArea(smb_buffer_response)
- <= BCC(smb_buffer_response)) {
+ <= get_unaligned((u16 *) ((char *)smb_buffer_response + sizeof(struct
smb_hdr) + (2 *
smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/) {
if (ses->serverOS)
kfree(ses->serverOS);
ses->serverOS =
@@ -3296,11 +3297,11 @@  CIFSNTLMSSPAuthSessSetup(unsigned int xi
if (smb_buffer->Flags2 & SMBFLG2_UNICODE) {
if ((long) (bcc_ptr) % 2) {
remaining_words =
- (BCC(smb_buffer_response)
+ (get_unaligned((u16 *) ((char *)smb_buffer_response + sizeof(struct
smb_hdr) + (2 *
smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/
- 1) / 2;
bcc_ptr++; /* Unicode strings must be word aligned */
} else {
- remaining_words = BCC(smb_buffer_response) / 2;
+ remaining_words = get_unaligned((u16 *) ((char *)smb_buffer_response +
sizeof(struct smb_hdr) + (2 *
smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/ / 2;
}
len = UniStrnlen((wchar_t *) bcc_ptr,
remaining_words - 1);
@@ -3384,7 +3385,7 @@  CIFSNTLMSSPAuthSessSetup(unsigned int xi
len = strnlen(bcc_ptr, 1024);
if (((long) bcc_ptr + len) -
(long) pByteArea(smb_buffer_response)
- <= BCC(smb_buffer_response)) {
+ <= get_unaligned((u16 *) ((char *)smb_buffer_response + sizeof(struct
smb_hdr) + (2 *
smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/) {
if (ses->serverOS)
kfree(ses->serverOS);
ses->serverOS = kzalloc(len + 1, GFP_KERNEL);
@@ -3542,7 +3543,7 @@  CIFSTCon(unsigned int xid, struct cifsSe
tcon->need_reconnect = false;
tcon->tid = smb_buffer_response->Tid;
bcc_ptr = pByteArea(smb_buffer_response);
- length = strnlen(bcc_ptr, BCC(smb_buffer_response) - 2);
+ length = strnlen(bcc_ptr, get_unaligned((u16 *) ((char
*)smb_buffer_response + sizeof(struct smb_hdr) + (2 *
smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/ - 2);
/* skip service field (NB: this field is always ASCII) */
if (length == 3) {
if ((bcc_ptr[0] == 'I') && (bcc_ptr[1] == 'P') &&
@@ -3562,7 +3563,7 @@  CIFSTCon(unsigned int xid, struct cifsSe
length = UniStrnlen((wchar_t *) bcc_ptr, 512);
if ((bcc_ptr + (2 * length)) -
pByteArea(smb_buffer_response) <=
- BCC(smb_buffer_response)) {
+ get_unaligned((u16 *) ((char *)smb_buffer_response + sizeof(struct
smb_hdr) + (2 *
smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/) {
kfree(tcon->nativeFileSystem);
tcon->nativeFileSystem =
kzalloc(2*(length + 1), GFP_KERNEL);
@@ -3581,7 +3582,7 @@  CIFSTCon(unsigned int xid, struct cifsSe
length = strnlen(bcc_ptr, 1024);
if ((bcc_ptr + length) -
pByteArea(smb_buffer_response) <=
- BCC(smb_buffer_response)) {
+ get_unaligned((u16 *) ((char *)smb_buffer_response + sizeof(struct
smb_hdr) + (2 *
smb_buffer_response->WordCount)))/*BCC(smb_buffer_response)*/) {
kfree(tcon->nativeFileSystem);
tcon->nativeFileSystem =
kzalloc(length + 1, GFP_KERNEL);
diff -rupw a/fs/cifs/netmisc.c b/fs/cifs/netmisc.c
--- a/fs/cifs/netmisc.c 2008-12-19 10:29:33.000000000 -0700
+++ b/fs/cifs/netmisc.c 2009-06-25 09:28:36.000000000 -0600
@@ -29,6 +29,7 @@ 
#include <linux/fs.h>
#include <asm/div64.h>
#include <asm/byteorder.h>
+#include <asm/unaligned.h>
#include <linux/inet.h>
#include "cifsfs.h"
#include "cifspdu.h"
@@ -840,11 +841,12 @@  smbCalcSize(struct smb_hdr *ptr)
2 /* size of the bcc field */ + BCC(ptr));
}

+
unsigned int
smbCalcSize_LE(struct smb_hdr *ptr)
{
return (sizeof(struct smb_hdr) + (2 * ptr->WordCount) +
- 2 /* size of the bcc field */ + le16_to_cpu(BCC_LE(ptr)));
+ 2 /* size of the bcc field */ + get_unaligned_le16((char *)ptr +
sizeof(struct smb_hdr) + (2 * ptr->WordCount))
/*le16_to_cpu(BCC_LE(ptr)) */ );
}

/* The following are taken from fs/ntfs/util.c */
diff -rupw a/fs/cifs/sess.c b/fs/cifs/sess.c
--- a/fs/cifs/sess.c 2009-06-25 16:55:30.000000000 -0600
+++ b/fs/cifs/sess.c 2009-06-25 13:51:36.000000000 -0600
@@ -29,6 +29,7 @@ 
#include "ntlmssp.h"
#include "nterr.h"
#include <linux/utsname.h>
+#include <asm/unaligned.h>
#include "cifs_spnego.h"

extern void SMBNTencrypt(unsigned char *passwd, unsigned char *c8,
@@ -131,10 +132,10 @@  static void unicode_ssetup_strings(char 
than 335 or will need to send them as arrays */

/* unicode strings, must be word aligned before the call */
-/* if ((long) bcc_ptr % 2) {
+ if ((long) bcc_ptr % 2) {
*bcc_ptr = 0;
bcc_ptr++;
- } */
+ }
/* copy user */
if (ses->userName == NULL) {
/* null user mount */
@@ -202,27 +203,26 @@  static int decode_unicode_ssetup(char **
int words_left, len;
char *data = *pbcc_area;

-
-
cFYI(1, ("bleft %d", bleft));

-
- /* SMB header is unaligned, so cifs servers word align start of
- Unicode strings */
- data++;
- bleft--; /* Windows servers do not always double null terminate
- their final Unicode string - in which case we
- now will not attempt to decode the byte of junk
- which follows it */
+ /*
+ * Windows servers do not always double null terminate their final
+ * Unicode string. Check to see if there are an uneven number of bytes
+ * left. If so, then add an extra NULL pad byte to the end of the
+ * response.
+ *
+ * See section 2.7.2 in "Implementing CIFS" for details
+ */
+ if (bleft % 2) {
+ data[bleft] = 0;
+ ++bleft;
+ }

words_left = bleft / 2;

/* save off server operating system */
len = UniStrnlen((wchar_t *) data, words_left);

-/* We look for obvious messed up bcc or strings in response so we do
not go off
- the end since (at least) WIN2K and Windows XP have a major bug in not
null
- terminating last Unicode string in response */
if (len >= words_left)