[PATCH] Fix an inproper alignment accessing in irda protocol stack

[PATCH] Fix an inproper alignment accessing in irda protocol stack

[Luke Yang]

[-- Attachment #1: Type: text/plain, Size: 1335 bytes --]

Hi all,

 For "struct irda_device_info" in irda.h:
struct irda_device_info {
	__u32       saddr;    /* Address of local interface */
	__u32       daddr;    /* Address of remote device */
	char        info[22]; /* Description */
	__u8        charset;  /* Charset used for description */
	__u8        hints[2]; /* Hint bits */
};
   The "hints" member aligns at the third byte of a word, an odd
address. So if we visit "hints" as a short in irlmp.c:

    u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;

  will cause alignment problem on some machines. Architectures with
strict alignment rules do not allow 16-bit read on an odd address.

Signed-off-by: Luke Yang <luke.adi@gmail.com>

 irlmp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

--- net/irda/irlmp.c.old        2006-06-08 14:49:20.000000000 +0800
+++ net/irda/irlmp.c    2006-06-08 14:54:29.000000000 +0800
@@ -849,7 +849,8 @@
        }

        /* Construct new discovery info to be used by IrLAP, */
-       u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
+       irlmp->discovery_cmd.data.hints[0] = irlmp->hints.word & 0xff;
+       irlmp->discovery_cmd.data.hints[1] = (irlmp->hints.word & 0xff00) >> 8;

        /*
         *  Set character set for device name (we use ASCII), and

-- 
Best regards,
Luke Yang
luke.adi@gmail.com

[-- Attachment #2: irlmp_alignment_fixing.patch --]
[-- Type: text/x-patch, Size: 470 bytes --]

--- net/irda/irlmp.c.old	2006-06-08 14:49:20.000000000 +0800
+++ net/irda/irlmp.c	2006-06-08 14:54:29.000000000 +0800
@@ -849,7 +849,8 @@
 	}
 
 	/* Construct new discovery info to be used by IrLAP, */
-	u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
+	irlmp->discovery_cmd.data.hints[0] = irlmp->hints.word & 0xff;
+	irlmp->discovery_cmd.data.hints[1] = (irlmp->hints.word & 0xff00) >> 8;
 
 	/*
 	 *  Set character set for device name (we use ASCII), and

Re: [PATCH] Fix an inproper alignment accessing in irda protocol stack

[Andrew Morton]

On Thu, 8 Jun 2006 15:15:11 +0800
"Luke Yang" <luke.adi@gmail.com> wrote:

> Hi all,
> 
>  For "struct irda_device_info" in irda.h:
> struct irda_device_info {
> 	__u32       saddr;    /* Address of local interface */
> 	__u32       daddr;    /* Address of remote device */
> 	char        info[22]; /* Description */
> 	__u8        charset;  /* Charset used for description */
> 	__u8        hints[2]; /* Hint bits */
> };
>    The "hints" member aligns at the third byte of a word, an odd
> address. So if we visit "hints" as a short in irlmp.c:
> 
>     u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
> 
>   will cause alignment problem on some machines. Architectures with
> strict alignment rules do not allow 16-bit read on an odd address.
> 
> Signed-off-by: Luke Yang <luke.adi@gmail.com>
> 
>  irlmp.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletion(-)
> 
> --- net/irda/irlmp.c.old        2006-06-08 14:49:20.000000000 +0800
> +++ net/irda/irlmp.c    2006-06-08 14:54:29.000000000 +0800
> @@ -849,7 +849,8 @@
>         }
> 
>         /* Construct new discovery info to be used by IrLAP, */
> -       u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
> +       irlmp->discovery_cmd.data.hints[0] = irlmp->hints.word & 0xff;
> +       irlmp->discovery_cmd.data.hints[1] = (irlmp->hints.word & 0xff00) >> 8;

This change will have the effect of swapping those two bytes on big-endian
machines.

Re: [PATCH] Fix an inproper alignment accessing in irda protocol stack

[Luke Yang]

[-- Attachment #1: Type: text/plain, Size: 2586 bytes --]

Hi Andrew,

   Thanks. I modified the patch (don't know if there is any way better
to solve this).

Signed-off-by: Luke Yang <luke.adi@gmail.com>

 irlmp.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletion(-)

--- net/irda/irlmp.c.old        2006-06-08 14:49:20.000000000 +0800
+++ net/irda/irlmp.c    2006-06-09 19:43:58.000000000 +0800
@@ -849,7 +849,13 @@
        }

        /* Construct new discovery info to be used by IrLAP, */
-       u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
+#ifdef __LITTLE_ENDIAN
+       irlmp->discovery_cmd.data.hints[0] = irlmp->hints.word & 0xff;
+       irlmp->discovery_cmd.data.hints[1] = (irlmp->hints.word & 0xff00) >> 8;
+#else /* ifdef __BIG_ENDIAN */
+       irlmp->discovery_cmd.data.hints[0] = (irlmp->hints.word & 0xff00) >> 8;
+       irlmp->discovery_cmd.data.hints[1] = irlmp->hints.word & 0xff;
+#endif

        /*
         *  Set character set for device name (we use ASCII), and

On 6/8/06, Andrew Morton <akpm@osdl.org> wrote:
> On Thu, 8 Jun 2006 15:15:11 +0800
> "Luke Yang" <luke.adi@gmail.com> wrote:
>
> > Hi all,
> >
> >  For "struct irda_device_info" in irda.h:
> > struct irda_device_info {
> >       __u32       saddr;    /* Address of local interface */
> >       __u32       daddr;    /* Address of remote device */
> >       char        info[22]; /* Description */
> >       __u8        charset;  /* Charset used for description */
> >       __u8        hints[2]; /* Hint bits */
> > };
> >    The "hints" member aligns at the third byte of a word, an odd
> > address. So if we visit "hints" as a short in irlmp.c:
> >
> >     u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
> >
> >   will cause alignment problem on some machines. Architectures with
> > strict alignment rules do not allow 16-bit read on an odd address.
> >
> > Signed-off-by: Luke Yang <luke.adi@gmail.com>
> >
> >  irlmp.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletion(-)
> >
> > --- net/irda/irlmp.c.old        2006-06-08 14:49:20.000000000 +0800
> > +++ net/irda/irlmp.c    2006-06-08 14:54:29.000000000 +0800
> > @@ -849,7 +849,8 @@
> >         }
> >
> >         /* Construct new discovery info to be used by IrLAP, */
> > -       u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
> > +       irlmp->discovery_cmd.data.hints[0] = irlmp->hints.word & 0xff;
> > +       irlmp->discovery_cmd.data.hints[1] = (irlmp->hints.word & 0xff00) >> 8;
>
> This change will have the effect of swapping those two bytes on big-endian
> machines.
>
>


-- 
Best regards,
Luke Yang
luke.adi@gmail.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: irlmp_alignment_fixing.patch --]
[-- Type: text/x-patch; name="irlmp_alignment_fixing.patch", Size: 674 bytes --]

--- net/irda/irlmp.c.old	2006-06-08 14:49:20.000000000 +0800
+++ net/irda/irlmp.c	2006-06-09 19:43:58.000000000 +0800
@@ -849,7 +849,13 @@
 	}
 
 	/* Construct new discovery info to be used by IrLAP, */
-	u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
+#ifdef __LITTLE_ENDIAN
+	irlmp->discovery_cmd.data.hints[0] = irlmp->hints.word & 0xff;
+	irlmp->discovery_cmd.data.hints[1] = (irlmp->hints.word & 0xff00) >> 8;
+#else /* ifdef __BIG_ENDIAN */
+	irlmp->discovery_cmd.data.hints[0] = (irlmp->hints.word & 0xff00) >> 8;
+	irlmp->discovery_cmd.data.hints[1] = irlmp->hints.word & 0xff;
+#endif
 
 	/*
 	 *  Set character set for device name (we use ASCII), and

Re: [PATCH] Fix an inproper alignment accessing in irda protocol stack

[David Miller]

From: "Luke Yang" <luke.adi@gmail.com>
Date: Fri, 9 Jun 2006 11:45:06 +0800

>         /* Construct new discovery info to be used by IrLAP, */
> -       u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
> +#ifdef __LITTLE_ENDIAN
> +       irlmp->discovery_cmd.data.hints[0] = irlmp->hints.word & 0xff;
> +       irlmp->discovery_cmd.data.hints[1] = (irlmp->hints.word & 0xff00) >> 8;
> +#else /* ifdef __BIG_ENDIAN */
> +       irlmp->discovery_cmd.data.hints[0] = (irlmp->hints.word & 0xff00) >> 8;
> +       irlmp->discovery_cmd.data.hints[1] = irlmp->hints.word & 0xff;
> +#endif

Please don't add ugly ifdefs, they are not necessary.

You can use the le16_to_cpu() macro on the hints.word datum,
then pick out the byte you need, as necessary.  That way you
don't need to use ifdefs.

Re: [PATCH] Fix an inproper alignment accessing in irda protocol stack

[Luke Yang]

[-- Attachment #1: Type: text/plain, Size: 1467 bytes --]

Hi all,

 For "struct irda_device_info" in irda.h:
struct irda_device_info {
       __u32       saddr;    /* Address of local interface */
       __u32       daddr;    /* Address of remote device */
       char        info[22]; /* Description */
       __u8        charset;  /* Charset used for description */
       __u8        hints[2]; /* Hint bits */
};
  The "hints" member aligns at the third byte of a word, an odd
address. So if we visit "hints" as a short in irlmp.c:

   u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;

 will cause alignment problem on some machines. Architectures with
strict alignment rules do not allow 16-bit read on an odd address. I
use le16_to_cpu to do the converting.

Signed-off-by: Luke Yang <luke.adi@gmail.com>

 irlmp.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletion(-)

--- net/irda/irlmp.c.old        2006-06-08 14:49:20.000000000 +0800
+++ net/irda/irlmp.c    2006-06-14 10:00:22.000000000 +0800
@@ -849,7 +849,10 @@
        }

        /* Construct new discovery info to be used by IrLAP, */
-       u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
+       irlmp->discovery_cmd.data.hints[0] = \
+               le16_to_cpu(irlmp->hints.word) & 0xff;
+       irlmp->discovery_cmd.data.hints[1] = \
+               (le16_to_cpu(irlmp->hints.word) & 0xff00) >> 8;

        /*
         *  Set character set for device name (we use ASCII), and

-- 
Best regards,
Luke Yang
luke.adi@gmail.com

[-- Attachment #2: irlmp_alignment_fixing.patch --]
[-- Type: text/x-patch, Size: 507 bytes --]

--- net/irda/irlmp.c.old	2006-06-08 14:49:20.000000000 +0800
+++ net/irda/irlmp.c	2006-06-14 10:00:22.000000000 +0800
@@ -849,7 +849,10 @@
 	}
 
 	/* Construct new discovery info to be used by IrLAP, */
-	u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
+	irlmp->discovery_cmd.data.hints[0] = \
+		le16_to_cpu(irlmp->hints.word) & 0xff;
+	irlmp->discovery_cmd.data.hints[1] = \
+		(le16_to_cpu(irlmp->hints.word) & 0xff00) >> 8;
 
 	/*
 	 *  Set character set for device name (we use ASCII), and

Re: [PATCH] Fix an inproper alignment accessing in irda protocol stack

[David Miller]

From: "Luke Yang" <luke.adi@gmail.com>
Date: Wed, 14 Jun 2006 10:29:19 +0800

>  For "struct irda_device_info" in irda.h:
> struct irda_device_info {
>        __u32       saddr;    /* Address of local interface */
>        __u32       daddr;    /* Address of remote device */
>        char        info[22]; /* Description */
>        __u8        charset;  /* Charset used for description */
>        __u8        hints[2]; /* Hint bits */
> };
>   The "hints" member aligns at the third byte of a word, an odd
> address. So if we visit "hints" as a short in irlmp.c:
> 
>    u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
> 
>  will cause alignment problem on some machines. Architectures with
> strict alignment rules do not allow 16-bit read on an odd address. I
> use le16_to_cpu to do the converting.
> 
> Signed-off-by: Luke Yang <luke.adi@gmail.com>

This looks good, I will apply it, thanks a lot.

Re: [PATCH] Fix an inproper alignment accessing in irda protocol stack

[David Miller]

From: "Luke Yang" <luke.adi@gmail.com>
Date: Wed, 14 Jun 2006 10:29:19 +0800

> --- net/irda/irlmp.c.old        2006-06-08 14:49:20.000000000 +0800
> +++ net/irda/irlmp.c    2006-06-14 10:00:22.000000000 +0800
> @@ -849,7 +849,10 @@
>         }
> 
>         /* Construct new discovery info to be used by IrLAP, */
> -       u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
> +       irlmp->discovery_cmd.data.hints[0] = \
> +               le16_to_cpu(irlmp->hints.word) & 0xff;
> +       irlmp->discovery_cmd.data.hints[1] = \
> +               (le16_to_cpu(irlmp->hints.word) & 0xff00) >> 8;
> 
>         /*
>          *  Set character set for device name (we use ASCII), and

I decided in the end to fix this differently.

We have a portable unaligned access interface, via get_unaligned() and
put_unaligned() in asm/unaligned.h, which makes sure there is no
penalty for platforms whose cpu does unaligned memory accesses
transparently.

diff --git a/net/irda/irlmp.c b/net/irda/irlmp.c
index c19e9ce..57ea160 100644
--- a/net/irda/irlmp.c
+++ b/net/irda/irlmp.c
@@ -44,6 +44,8 @@
 #include <net/irda/irlmp.h>
 #include <net/irda/irlmp_frame.h>
 
+#include <asm/unaligned.h>
+
 static __u8 irlmp_find_free_slsap(void);
 static int irlmp_slsap_inuse(__u8 slsap_sel);
 
@@ -840,6 +842,7 @@ void irlmp_do_expiry(void)
 void irlmp_do_discovery(int nslots)
 {
 	struct lap_cb *lap;
+	__u16 *data_hintsp;
 
 	/* Make sure the value is sane */
 	if ((nslots != 1) && (nslots != 6) && (nslots != 8) && (nslots != 16)){
@@ -849,7 +852,8 @@ void irlmp_do_discovery(int nslots)
 	}
 
 	/* Construct new discovery info to be used by IrLAP, */
-	u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
+	data_hintsp = (__u16 *) irlmp->discovery_cmd.data.hints;
+	put_unaligned(irlmp->hints.word, data_hintsp);
 
 	/*
 	 *  Set character set for device name (we use ASCII), and

Re: [PATCH] Fix an inproper alignment accessing in irda protocol stack

[Luke Yang]

Hi all,

  There is another same unaligend issue in irda stack to be fixed:

Signed-off-by: Luke Yang <luke.adi@gmail.com>

--- linux-2.6.x/net/irda/discovery.c    2006-03-22 18:32:37.000000000 +0800
+++ ../../uClinux-dist/linux-2.6.x/net/irda/discovery.c 2006-06-30
18:07:46.000000000 +0800
@@ -38,6 +38,7 @@
 #include <net/irda/irlmp.h>

 #include <net/irda/discovery.h>
+#include <asm/unaligned.h>

 /*
  * Function irlmp_add_discovery (cachelog, discovery)
@@ -86,7 +87,7 @@
                         */
                        hashbin_remove_this(cachelog, (irda_queue_t *) node);
                        /* Check if hints bits are unchanged */
-                       if(u16ho(node->data.hints) == u16ho(new->data.hints))
+                       if(get_unaligned(node->data.hints) ==
get_unaligned(new->data.hints))
                                /* Set time of first discovery for this node */
                                new->firststamp = node->firststamp;
                        kfree(node);
@@ -280,7 +281,7 @@
                /* Mask out the ones we don't want :
                 * We want to match the discovery mask, and to get only
                 * the most recent one (unless we want old ones) */
-               if ((u16ho(discovery->data.hints) & mask) &&
+               if ((get_unaligned(discovery->data.hints) & mask) &&
                    ((old_entries) ||
                     ((jiffies - discovery->firststamp) < j_timeout)) ) {
                        /* Create buffer as needed.


Regards,
Luke Yang

On 6/18/06, David Miller <davem@davemloft.net> wrote:
> From: "Luke Yang" <luke.adi@gmail.com>
> Date: Wed, 14 Jun 2006 10:29:19 +0800
>
> > --- net/irda/irlmp.c.old        2006-06-08 14:49:20.000000000 +0800
> > +++ net/irda/irlmp.c    2006-06-14 10:00:22.000000000 +0800
> > @@ -849,7 +849,10 @@
> >         }
> >
> >         /* Construct new discovery info to be used by IrLAP, */
> > -       u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
> > +       irlmp->discovery_cmd.data.hints[0] = \
> > +               le16_to_cpu(irlmp->hints.word) & 0xff;
> > +       irlmp->discovery_cmd.data.hints[1] = \
> > +               (le16_to_cpu(irlmp->hints.word) & 0xff00) >> 8;
> >
> >         /*
> >          *  Set character set for device name (we use ASCII), and
>
> I decided in the end to fix this differently.
>
> We have a portable unaligned access interface, via get_unaligned() and
> put_unaligned() in asm/unaligned.h, which makes sure there is no
> penalty for platforms whose cpu does unaligned memory accesses
> transparently.
>
> diff --git a/net/irda/irlmp.c b/net/irda/irlmp.c
> index c19e9ce..57ea160 100644
> --- a/net/irda/irlmp.c
> +++ b/net/irda/irlmp.c
> @@ -44,6 +44,8 @@
>  #include <net/irda/irlmp.h>
>  #include <net/irda/irlmp_frame.h>
>
> +#include <asm/unaligned.h>
> +
>  static __u8 irlmp_find_free_slsap(void);
>  static int irlmp_slsap_inuse(__u8 slsap_sel);
>
> @@ -840,6 +842,7 @@ void irlmp_do_expiry(void)
>  void irlmp_do_discovery(int nslots)
>  {
>         struct lap_cb *lap;
> +       __u16 *data_hintsp;
>
>         /* Make sure the value is sane */
>         if ((nslots != 1) && (nslots != 6) && (nslots != 8) && (nslots != 16)){
> @@ -849,7 +852,8 @@ void irlmp_do_discovery(int nslots)
>         }
>
>         /* Construct new discovery info to be used by IrLAP, */
> -       u16ho(irlmp->discovery_cmd.data.hints) = irlmp->hints.word;
> +       data_hintsp = (__u16 *) irlmp->discovery_cmd.data.hints;
> +       put_unaligned(irlmp->hints.word, data_hintsp);
>
>         /*
>          *  Set character set for device name (we use ASCII), and
>


-- 
Best regards,
Luke Yang
luke.adi@gmail.com

Re: [PATCH] Fix an inproper alignment accessing in irda protocol stack

[David Miller]

From: "Luke Yang" <luke.adi@gmail.com>
Date: Tue, 11 Jul 2006 10:12:41 +0800

>   There is another same unaligend issue in irda stack to be fixed:
> 
> Signed-off-by: Luke Yang <luke.adi@gmail.com>

Your patch is corrupted by your email client and cannot be applied
cleanly to the current kernel sources.

This is the second time around I've had to ask you to correct this
kind of problem with your submissions.  Consider this my last and
final warning.

A lot of my time is wasted when patches are improperly submitted.

Re: [PATCH] Fix an inproper alignment accessing in irda protocol stack

[Luke Yang]

On 7/11/06, David Miller <davem@davemloft.net> wrote:
> From: "Luke Yang" <luke.adi@gmail.com>
> Date: Tue, 11 Jul 2006 10:12:41 +0800
>
> >   There is another same unaligend issue in irda stack to be fixed:
> >
> > Signed-off-by: Luke Yang <luke.adi@gmail.com>
>
> Your patch is corrupted by your email client and cannot be applied
> cleanly to the current kernel sources.
>
> This is the second time around I've had to ask you to correct this
> kind of problem with your submissions.  Consider this my last and
> final warning.
>
> A lot of my time is wasted when patches are improperly submitted.

 I am sorry. I have resend the whole patch for this issue in right
format. And as gmail keeps convert tabs to spaces. I attached the
right format patch as a attachment. So you can read the patch in my
mail and use the attached one to check in. This method has been
discussed and acceptted.
>


-- 
Best regards,
Luke Yang
luke.adi@gmail.com

Re: [PATCH] Fix an inproper alignment accessing in irda protocol stack

[David Miller]

From: "Luke Yang" <luke.adi@gmail.com>
Date: Tue, 11 Jul 2006 12:19:46 +0800

>  I am sorry. I have resend the whole patch for this issue in right
> format. And as gmail keeps convert tabs to spaces. I attached the
> right format patch as a attachment. So you can read the patch in my
> mail and use the attached one to check in. This method has been
> discussed and acceptted.

Fine, please resend your patches, but this time to the
appropriate mailing list, which is netdev@vger.kernel.org

Thank you.