Compatibility issue with 2.2.19pre7

Compatibility issue with 2.2.19pre7

[Hubert Mantel]

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

Hi,

is this part of 2.2.19pre7 really a good idea? Even in 2.4.0 the size
field is still a short.
                                                                  -o)
    Hubert Mantel              Goodbye, dots...                   /\\
                                                                 _\_v

[-- Attachment #2: pre-patch-2.2.19-7 --]
[-- Type: text/plain, Size: 408 bytes --]

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla/include/linux/nfs.h linux.19p7/include/linux/nfs.h
--- linux.vanilla/include/linux/nfs.h	Mon Dec 11 22:13:07 2000
+++ linux.19p7/include/linux/nfs.h	Wed Jan  3 00:58:38 2001
@@ -94,7 +94,7 @@
  */
 #define NFS_MAXFHSIZE		64
 struct nfs_fh {
-	unsigned short		size;
+	unsigned int		size;
 	unsigned char		data[NFS_MAXFHSIZE];
 };
 

Re: Compatibility issue with 2.2.19pre7

[Alan Cox]

> is this part of 2.2.19pre7 really a good idea? Even in 2.4.0 the size
> field is still a short.

It needs to change in 2.4 as well. The cast of data to a struct isnt portable
to all architectures.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Russell King]

Hubert Mantel writes:
> is this part of 2.2.19pre7 really a good idea? Even in 2.4.0 the size
> field is still a short.
>  #define NFS_MAXFHSIZE		64
>  struct nfs_fh {
> -	unsigned short		size;
> +	unsigned int		size;
>  	unsigned char		data[NFS_MAXFHSIZE];
>  };

This is an internal kernel data structure.  Do you know of some program
that breaks as a result of this?
   _____
  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        rmk@arm.linux.org.uk      --- ---
  | | | | http://www.arm.linux.org.uk/personal/aboutme.html   /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Andrea Arcangeli]

On Wed, Jan 10, 2001 at 06:54:45AM +0000, Russell King wrote:
> This is an internal kernel data structure.  Do you know of some program

No, it isn't, that's the whole point.

> that breaks as a result of this?

(spotted by Andi) util-linux-2.10o/mount/nfs_mount4.h:

struct nfs3_fh {
        unsigned short          size;
        unsigned char           data[64];
};

(see also nfs_mount_data structure in both kernel and mount)

I also don't understand Alan's comment, what has the cast of data to a
structure have to do with the size of a field in the structure? Furthmore the
cast of data to a struct should work on all architectures as far as C is
concerned (if you then run alignment problems then it's your mistake).

As far I can see the only reason size makes sense to be 32bit is to get some
more strict behaviour in the below code (to avoid discarding the most
significant 16bits in sanity checks like this):

nlm4_decode_fh(u32 *p, struct nfs_fh *f)
{
        memset(f->data, 0, sizeof(f->data));
        f->size = ntohl(*p++);
        if (f->size > NFS_MAXFHSIZE) {
                printk(KERN_NOTICE
                        "lockd: bad fhandle size %d (should be <=%d)\n",
                        f->size, NFS_MAXFHSIZE);
                return NULL;
        }
        memcpy(f->data, p, f->size);
        return p + XDR_QUADLEN(f->size);
}

So for now I backed it out. If you want to push it in again then implement it
right and make an mount backwards compatible nfs_fh type for the
nfs_mount_data.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Russell King]

Andrea Arcangeli writes:
> (spotted by Andi) util-linux-2.10o/mount/nfs_mount4.h:
> 
> struct nfs3_fh {
>         unsigned short          size;
>         unsigned char           data[64];
> };
> 
> (see also nfs_mount_data structure in both kernel and mount)

Well, let me put it this way:

1. NFS locking worked on ARM in 2.2.17 without any problems or modifications
   what so ever.

2. NFS locking does NOT work on ARM without the above patch.  It FAILS
   completely.

> I also don't understand Alan's comment, what has the cast of data to a
> structure have to do with the size of a field in the structure?

I won't bother replying to this, since you obviously know the answer from
your following comment. ;)

> Furthmore
> the cast of data to a struct should work on all architectures as far as C is
> concerned (if you then run alignment problems then it's your mistake).

WRONG WRONG WRONG WRONG WRONG.  You are *SO* wrong.

C explicitly allows this behaviour.  The fact that most processors can load
unaligned data without any trouble is merely co-incidence and leads to BAD
programming techniques, like this one illusatrated above.

Therefore, it is BAD code which needs to be fixed.  Now, on ARM, all
structures are defined by the ABI to be aligned to a 32-bit word.  End
of story.  No other story will do.  Finito.  Capisco?

Yes, you can get around them by taking an alignment abort, but that is REALLY
REALLY expensive.  Eg, instead of a load taking 1 cycle, it turns into around
500 cycles, just to decode the instruction by hand, calculate the addresses,
calculate the side effects and implement them.

So, if we can get away with a little BETTER coding technique, then that is
FAR FAR better than thaning a 499 cycle penalty.

Oh, and did I mention that x86 also has a performance penalty for unaligned
loads as well?

> So for now I backed it out. If you want to push it in again then implement
> it right and make an mount backwards compatible nfs_fh type for the
> nfs_mount_data.

This is pretty gawling, especially as the fix was posted to the NFS
development list and not one person complained that it would break any APIs.

I'd like the NFS developers to look at this.  Yes, I'm delegating it.  Why?
I've got too much on my plate at the moment, and I'm desperately trying to
get rid of the 2.2 ARM kernel tree so I can concentrate on 2.4 only.  In the
spirit of open source, I am willing to test patches that claim to fix this
problem once I've got my 2.2 kernel tree back into a reasonable state (after
chasing some 128MB bad DIMM problem around for 2 months across 2 versions of
Linux).

And yes, APIs shouldn't break in a major kernel release.  Its a shame some
API broke in 2.2.18.
   _____
  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        rmk@arm.linux.org.uk      --- ---
  | | | | http://www.arm.linux.org.uk/personal/aboutme.html   /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Andrea Arcangeli]

On Wed, Jan 10, 2001 at 10:09:22PM +0000, Russell King wrote:
> Andrea Arcangeli writes:
> > Furthmore
> > the cast of data to a struct should work on all architectures as far as C is
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > concerned (if you then run alignment problems then it's your mistake).
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> WRONG WRONG WRONG WRONG WRONG.  You are *SO* wrong.
> 
> C explicitly allows this behaviour.  The fact that most processors can load
> unaligned data without any trouble is merely co-incidence and leads to BAD
> programming techniques, like this one illusatrated above.
> 
> Therefore, it is BAD code which needs to be fixed.  Now, on ARM, all
> structures are defined by the ABI to be aligned to a 32-bit word.  End
> of story.  No other story will do.  Finito.  Capisco?
> 
> Yes, you can get around them by taking an alignment abort, but that is REALLY
> REALLY expensive.  Eg, instead of a load taking 1 cycle, it turns into around
> 500 cycles, just to decode the instruction by hand, calculate the addresses,
> calculate the side effects and implement them.

Sure, also alpha and sparc faults on misaligned accesses (on alpha we're nicer
because we emulate them in kernel most of the time). I'm not missing those
alignment issues.

Read again and you'll see that what I said isn't that ARM is broken because
doesn't handle misaligned accesses. What I said is that I can write this C
code:

	int x[2], * p = (int *) (((char *) &x)+1);
	main()
	{
		*p = 0;
	}

This is legal C code. Try compiling this on sparc that bus error on the above
code. The cast works. Works fine. Compiler is happy.

Then when you execute the code the asm will happen to derefernce a misaligned
and it breaks at runtime.

But it's _not_ the cast that is wrong. It's the _usage_ of the cast that is
wrong, and it's your mistake not C's mistake or cast's mistake (and with "your"
I didn't mean Russell's, you obviously care about those issues ;).

Infact the usage of the same below cast will work fine on ARM and sparc too,
but now incidentally on an address that is properly aligned:

	int x[2], * p = (int *) (((char *) &x)+4);
	main()
	{
		*p = 0;
	}

Is it clear now what I said? Capito? ;)

> So, if we can get away with a little BETTER coding technique, then that is
> FAR FAR better than thaning a 499 cycle penalty.

I never suggest you should emulate this in kernel.

> Oh, and did I mention that x86 also has a performance penalty for unaligned
> loads as well?

I know.

> > So for now I backed it out. If you want to push it in again then implement
> > it right and make an mount backwards compatible nfs_fh type for the
> > nfs_mount_data.
> 
> This is pretty gawling, especially as the fix was posted to the NFS
> development list and not one person complained that it would break any APIs.

They missed it.

> I'd like the NFS developers to look at this.  Yes, I'm delegating it.  Why?

No problem, AFIK Andi will take care of it and he will implement the right fix.

However I'd _love_ to see the EIP where you get the fault, I currently don't
see the line of code that is crashing your ARM and I know this code doesn't
segfault on ARM.

        struct x {
                unsigned short x;
                unsigned char c[64];
        };
 
        main()
        {
                struct x x;
                x.x = 0;
		bzero(x.c, 64);	
        }

> And yes, APIs shouldn't break in a major kernel release.  Its a shame some
> API broke in 2.2.18.

What broke in 2.2.18?

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Russell King]

Andrea Arcangeli writes:
> However I'd _love_ to see the EIP where you get the fault, I currently don't
> see the line of code that is crashing your ARM and I know this code doesn't
> segfault on ARM.

Examine nlm_lookup_file() and the usage of fh->fh_dev and fh->fh_ino.
What happens is that in that dprintk fh_dev and fh_ino contain garbage
and as such, the nlm_lookup_file fails (since it doesn't refer to a valid
device and inode pair).  Therefore locking does not work.

Note: I've never said that it crashes.  I've only said that NFS locking
does not work.

> > And yes, APIs shouldn't break in a major kernel release.  Its a shame some
> > API broke in 2.2.18.
> 
> What broke in 2.2.18?

The API changed:

 struct nfs_mount_data {
        int             version;                /* 1 */
        int             fd;                     /* 1 */
-       struct nfs_fh   root;                   /* 1 */
+       struct nfs2_fh  old_root;               /* 1 */
        int             flags;                  /* 1 */
        int             rsize;                  /* 1 */
        int             wsize;                  /* 1 */
@@ -35,6 +42,7 @@
        char            hostname[256];          /* 1 */
        int             namlen;                 /* 2 */
        unsigned int    bsize;                  /* 3 */
+       struct nfs_fh   root;                   /* 4 */
 };

which then caused this breakage.  Therefore, I still propose my original
patch as a bug fix against something that appears to be brand new in
design.

Anyway, changing it back will break the API on ARM, so either way API
breakage WILL happen at some point.  The real question is which is better:

1. Yucky code in the NFS layers to copy a nfs_fh from userspace to kernel
   space, translating it into something sane.
2. Yucky code in the NFS layers to manually handle the nfs_fh to knfs_fh
   translation.
3. Accept the breakage of a *brand new* API in 2.2.18 and suffer zero
   yucky code.
   _____
  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        rmk@arm.linux.org.uk      --- ---
  | | | | http://www.arm.linux.org.uk/personal/aboutme.html   /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Andi Kleen]

On Thu, Jan 11, 2001 at 07:34:01AM +0000, Russell King wrote:
> 1. Yucky code in the NFS layers to copy a nfs_fh from userspace to kernel
>    space, translating it into something sane.
> 2. Yucky code in the NFS layers to manually handle the nfs_fh to knfs_fh
>    translation.
> 3. Accept the breakage of a *brand new* API in 2.2.18 and suffer zero
>    yucky code.

The change is rather useless anyways, because even in NFSv3 file handles
cannot be >64bytes. Would even fit in a char, doesn't need a short nor an
int. 

-Andi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Russell King]

Andi Kleen writes:
> The change is rather useless anyways, because even in NFSv3 file handles
> cannot be >64bytes. Would even fit in a char, doesn't need a short nor an
> int. 

Indeed, but whether it be a char or a short, it'll still break on ARM.  My
original set of 3 solutions still stand therefore.
   _____
  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        rmk@arm.linux.org.uk      --- ---
  | | | |   http://www.arm.linux.org.uk/~rmk/aboutme.html    /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Trond Myklebust]

>>>>> " " == Russell King <rmk@arm.linux.org.uk> writes:

     > Hubert Mantel writes:
    >> is this part of 2.2.19pre7 really a good idea? Even in 2.4.0
    >> the size field is still a short.
    >> #define NFS_MAXFHSIZE 64
    >> struct nfs_fh {
    >> - unsigned short size;
    >> + unsigned int size;
    >> unsigned char data[NFS_MAXFHSIZE]; };

     > This is an internal kernel data structure.  Do you know of some
     > program that breaks as a result of this?
     >    _____

Any program which mounts an NFS partition.

If you do this, then you need to provide some sort of compatibility
layer for nfs_mount.h since the format for version 4 of the NFS mount
structure was decided more than 2 years ago.

Cheers,
  Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Andi Kleen]

On Thu, Jan 11, 2001 at 07:10:27AM -0500, Manfred wrote:
> Zitiere Russell King <rmk@arm.linux.org.uk>:
> > The API changed:
> >  struct nfs_mount_data {
> >         int             version;                /* 1 */
> >         int             fd;                     /* 1 */
> > -       struct nfs_fh   root;                   /* 1 */
> > +       struct nfs2_fh  old_root;               /* 1 */
> 
> I don't see an API change:
> the 2.2.17 "struct nfs_fs" and the 2.2.18 "struct nfs2_fh" are identical.

But it changed in 2.2.19pre, breaking nfs mount on i386. 




-Andi

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Manfred]

Zitiere Russell King <rmk@arm.linux.org.uk>:
> The API changed:
>  struct nfs_mount_data {
>         int             version;                /* 1 */
>         int             fd;                     /* 1 */
> -       struct nfs_fh   root;                   /* 1 */
> +       struct nfs2_fh  old_root;               /* 1 */

I don't see an API change:
the 2.2.17 "struct nfs_fs" and the 2.2.18 "struct nfs2_fh" are identical.

Ok, I see the problem:

struct nfs_fh {
    unsigned short          size;
    unsigned char           data[NFS_MAXFHSIZE];
}

The compiler thinks that data is a character array, thus no padding is inserted.
nlm_lookup_file casts f->data to "struct knfs_fs", a structure with pointers and
u32. --> unaligned u32 read --> boom.

Is that correct?
Is &(((struct nfs_fh*)0)->data) 2 or 4?

ARM isn't the only cpu that can't handle unaligned memory reads, why doesn't the
code fail on Alpha/Sparc? Does gcc insert padding on these cpus?


--
	Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Manfred]

Zitiere Andi Kleen <ak@suse.de>:

> On Thu, Jan 11, 2001 at 07:10:27AM -0500, Manfred wrote:
> > Zitiere Russell King <rmk@arm.linux.org.uk>:
> > > The API changed:
> > >  struct nfs_mount_data {
> > >         int             version;                /* 1 */
> > >         int             fd;                     /* 1 */
> > > -       struct nfs_fh   root;                   /* 1 */
> > > +       struct nfs2_fh  old_root;               /* 1 */
> > 
> > I don't see an API change:
> > the 2.2.17 "struct nfs_fs" and the 2.2.18 "struct nfs2_fh" are
> identical.
> 
> But it changed in 2.2.19pre, breaking nfs mount on i386. 
> 

-ECONFUSED.

2.2.17 struct nfs_fh is identical to 2.2.18 nfs2_fh and 2.2.19pre7 nfs2_fh

2.2.18 struct nfs_fh is a new structure for nfsV3, it doesn't exist in 2.2.17.
That structure is unusable on ARM.

Russel want's to change the new "struct nfs_fh" (from 2.2.18), and that change
is included in 2.2.19pre7. But that change breaks i386 nfs mount.

Could someone with an Alpha/Sparc/ARM compiler compile a test program with
"struct nfs_fh" from 2.2.18 and print the offset of nfs_fh.data?

--
	Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Russell King]

Manfred writes:
> 2.2.18 struct nfs_fh is a new structure for nfsV3, it doesn't exist in 2.2.17.
> That structure is unusable on ARM.

Correct.

> Russel want's to change the new "struct nfs_fh" (from 2.2.18), and that
> change is included in 2.2.19pre7. But that change breaks i386 nfs mount.

Unfortunately.

> Could someone with an Alpha/Sparc/ARM compiler compile a test program with
> "struct nfs_fh" from 2.2.18 and print the offset of nfs_fh.data?

2
   _____
  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        rmk@arm.linux.org.uk      --- ---
  | | | |   http://www.arm.linux.org.uk/~rmk/aboutme.html    /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Trond Myklebust]

>>>>> " " == Andrea Arcangeli <andrea@suse.de> writes:

     > As far I can see the only reason size makes sense to be 32bit
     > is to get some more strict behaviour in the below code (to
     > avoid discarding the most significant 16bits in sanity checks
     > like this):

     > nlm4_decode_fh(u32 *p, struct nfs_fh *f) {
     >         memset(f->data, 0, sizeof(f->data));
     >         f-> size = ntohl(*p++);
     >         if (f->size > NFS_MAXFHSIZE) {
     >                 printk(KERN_NOTICE
     >                         "lockd: bad fhandle size %d (should be
     >                         <=%d)\n",
     >                         f-> size, NFS_MAXFHSIZE);
     >                 return NULL;
     >         }
     >         memcpy(f->data, p, f->size);
     >         return p + XDR_QUADLEN(f->size);
     > }

I agree, and if that's the only problem, then the appended patch will
fix it without any need to change struct nfs_fh.

As for the issue of casting 'fh->data' as a 'struct knfsd' then that
is a perfectly valid operation.
The fh->data is a cookie as far as the client is concerned, and hence
it will pass back exactly whatever the server sent it (alignment and
all).

IOW: the knfsd server copied a struct knfsd and sent it off to the
client, and now the exact same server is receiving a completely
unadulterated version of said struct knfsd for use by the lockd server
routines.
Unless somebody is using one compiler for the knfsd directory and then
a completely different one for lockd, I fail to see why this should
result in structure alignment problems on PPC or on any other
platform.

Cheers,
   Trond





--- fs/lockd/xdr4.c.orig	Thu Jan 11 15:52:44 2001
+++ fs/lockd/xdr4.c	Thu Jan 11 15:53:37 2001
@@ -83,16 +83,19 @@
 static u32 *
 nlm4_decode_fh(u32 *p, struct nfs_fh *f)
 {
+	unsigned int size;
+
 	memset(f->data, 0, sizeof(f->data));
-	f->size = ntohl(*p++);
-	if (f->size > NFS_MAXFHSIZE) {
+	size = ntohl(*p++);
+	if (size > NFS_MAXFHSIZE) {
 		printk(KERN_NOTICE
 			"lockd: bad fhandle size %x (should be %d)\n",
-			f->size, NFS_MAXFHSIZE);
+			size, NFS_MAXFHSIZE);
 		return NULL;
 	}
-      	memcpy(f->data, p, f->size);
-	return p + XDR_QUADLEN(f->size);
+	f->size = size;
+      	memcpy(f->data, p, size);
+	return p + XDR_QUADLEN(size);
 }
 
 static u32 *
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Manfred Spraul]

Trond Myklebust wrote:
> 
> 
> As for the issue of casting 'fh->data' as a 'struct knfsd' then that
> is a perfectly valid operation.
>
No it isn't.

fh->data is an array of characters, thus without any alignment
restrictions.
'struct knfsd' begins with a pointer, thus it must be 4 or 8 byte
aligned.

The portable 'struct nfs_fh' structure would be

#define NFS_HANDLESIZE	64
struct nfs_fh
{
	unsigned short len;
	void* data[NFS_HANDLESIZE/sizeof(void*)];
};

But now its too late for such a change - it breaks at least i386,
probably all platforms.

Does knfsd actually need all 64 bytes in the nfs_fh?
What about aligning the 'struct knfsd' manually?

-	struct knfsd* ptr = fh->data;
+	struct knfsd* ptr = (fh->data+15)&(~15);

That would be kernel only, no ABI problems.

--
	Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Trond Myklebust]

>>>>> " " == Manfred Spraul <manfred@colorfullife.com> writes:

     > Trond Myklebust wrote:
    >>
    >>
    >> As for the issue of casting 'fh->data' as a 'struct knfsd' then
    >> that is a perfectly valid operation.
    >>
     > No it isn't.

     > fh-> data is an array of characters, thus without any alignment
     > restrictions.  'struct knfsd' begins with a pointer, thus it
     > must be 4 or 8 byte aligned.

     > The portable 'struct nfs_fh' structure would be

     > #define NFS_HANDLESIZE 64
     > struct nfs_fh {
     > 	unsigned short len; void* data[NFS_HANDLESIZE/sizeof(void*)];
     > };

Ok. I see your point now. How about the appended patch then? It means
an extra copy operation, but it should be a lot less ugly than doing
manual alignment...

Cheers,
  Trond

diff -u --recursive --new-file linux-2.2.18/fs/lockd/svcsubs.c linux-2.2.18-fix_ppc/fs/lockd/svcsubs.c
--- linux-2.2.18/fs/lockd/svcsubs.c	Mon Dec 11 01:49:44 2000
+++ linux-2.2.18-fix_ppc/fs/lockd/svcsubs.c	Thu Jan 11 18:43:31 2001
@@ -49,34 +49,37 @@
 nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
 					struct nfs_fh *f)
 {
-	struct knfs_fh	*fh = (struct knfs_fh *) f->data;
+	struct knfs_fh	fh;
 	struct nlm_file	*file;
 	unsigned int	hash;
 	u32		nfserr;
 
+	/* Copy filehandle to avoid pointer alignment issues */
+	memcpy(&fh, f->data, sizeof(fh));
+
 	dprintk("lockd: nlm_file_lookup(%s/%u)\n",
-		kdevname(u32_to_kdev_t(fh->fh_dev)), fh->fh_ino);
+		kdevname(u32_to_kdev_t(fh.fh_dev)), fh.fh_ino);
 
-	hash = file_hash(u32_to_kdev_t(fh->fh_dev), u32_to_ino_t(fh->fh_ino));
+	hash = file_hash(u32_to_kdev_t(fh.fh_dev), u32_to_ino_t(fh.fh_ino));
 
 	/* Lock file table */
 	down(&nlm_file_sema);
 
 	for (file = nlm_files[hash]; file; file = file->f_next) {
-		if (file->f_handle.fh_dcookie == fh->fh_dcookie &&
-		    !memcmp(&file->f_handle, fh, sizeof(*fh)))
+		if (file->f_handle.fh_dcookie == fh.fh_dcookie &&
+		    !memcmp(&file->f_handle, &fh, sizeof(fh)))
 			goto found;
 	}
 
 	dprintk("lockd: creating file for %s/%u\n",
-		kdevname(u32_to_kdev_t(fh->fh_dev)), fh->fh_ino);
+		kdevname(u32_to_kdev_t(fh.fh_dev)), fh.fh_ino);
 	nfserr = nlm4_lck_denied_nolocks;
 	file = (struct nlm_file *) kmalloc(sizeof(*file), GFP_KERNEL);
 	if (!file)
 		goto out_unlock;
 
 	memset(file, 0, sizeof(*file));
-	file->f_handle = *fh;
+	memcpy(&file->f_handle, &fh, sizeof(file->f_handle));
 	file->f_sema   = MUTEX;
 
 	/* Open the file. Note that this must not sleep for too long, else
@@ -85,7 +88,7 @@
 	 * We have to make sure we have the right credential to open
 	 * the file.
 	 */
-	if ((nfserr = nlmsvc_ops->fopen(rqstp, fh, &file->f_file)) != 0) {
+	if ((nfserr = nlmsvc_ops->fopen(rqstp, &fh, &file->f_file)) != 0) {
 		dprintk("lockd: open failed (nfserr %d)\n", ntohl(nfserr));
 		goto out_free;
 	}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Trond Myklebust]

>>>>> " " == Trond Myklebust <trond.myklebust@fys.uio.no> writes:

     > - if (file->f_handle.fh_dcookie == fh->fh_dcookie &&
     > - !memcmp(&file->f_handle, fh, sizeof(*fh)))
     > + if (file->f_handle.fh_dcookie == fh.fh_dcookie &&
     > + !memcmp(&file->f_handle, &fh, sizeof(fh)))
     >  			goto found;

Come to think of it, this line looks pretty insane. Why on earth are
we testing fh_dcookie twice?

I suspect that just the elimination of the redundant comparison in the
above line would eliminate Russell's problem entirely, given that it's
the only place in the entire routine where we actually reference
fh->fh_base.fb_dentry.

In all other cases, we're referencing ordinary integers. Are there any
alignment requirements on them?

Cheers,
  Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Andrea Arcangeli]

On Thu, Jan 11, 2001 at 07:22:03PM +0100, Trond Myklebust wrote:
> [..] Are there any
> alignment requirements on them?

On some arch int can be read only at a sizeof(int) byte aligned address
(details in my example in reply to Russell).

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Trond Myklebust]

>>>>> " " == Andrea Arcangeli <andrea@suse.de> writes:

     > On Thu, Jan 11, 2001 at 07:22:03PM +0100, Trond Myklebust
     > wrote:
    >> [..] Are there any alignment requirements on them?

     > On some arch int can be read only at a sizeof(int) byte aligned
     > address (details in my example in reply to Russell).

OK. In that case my patch, would just be amended to eliminate the
redundant comparison as is the case below.

Cheers,
   Trond

diff -u --recursive --new-file linux-2.2.18/fs/lockd/svcsubs.c linux-2.2.18-fix_ppc/fs/lockd/svcsubs.c
--- linux-2.2.18/fs/lockd/svcsubs.c	Mon Dec 11 01:49:44 2000
+++ linux-2.2.18-fix_ppc/fs/lockd/svcsubs.c	Thu Jan 11 19:00:11 2001
@@ -49,34 +49,36 @@
 nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
 					struct nfs_fh *f)
 {
-	struct knfs_fh	*fh = (struct knfs_fh *) f->data;
+	struct knfs_fh	fh;
 	struct nlm_file	*file;
 	unsigned int	hash;
 	u32		nfserr;
 
+	/* Copy filehandle to avoid pointer alignment issues */
+	memcpy(&fh, f->data, sizeof(fh));
+
 	dprintk("lockd: nlm_file_lookup(%s/%u)\n",
-		kdevname(u32_to_kdev_t(fh->fh_dev)), fh->fh_ino);
+		kdevname(u32_to_kdev_t(fh.fh_dev)), fh.fh_ino);
 
-	hash = file_hash(u32_to_kdev_t(fh->fh_dev), u32_to_ino_t(fh->fh_ino));
+	hash = file_hash(u32_to_kdev_t(fh.fh_dev), u32_to_ino_t(fh.fh_ino));
 
 	/* Lock file table */
 	down(&nlm_file_sema);
 
 	for (file = nlm_files[hash]; file; file = file->f_next) {
-		if (file->f_handle.fh_dcookie == fh->fh_dcookie &&
-		    !memcmp(&file->f_handle, fh, sizeof(*fh)))
+		if (!memcmp(&file->f_handle, &fh, sizeof(fh)))
 			goto found;
 	}
 
 	dprintk("lockd: creating file for %s/%u\n",
-		kdevname(u32_to_kdev_t(fh->fh_dev)), fh->fh_ino);
+		kdevname(u32_to_kdev_t(fh.fh_dev)), fh.fh_ino);
 	nfserr = nlm4_lck_denied_nolocks;
 	file = (struct nlm_file *) kmalloc(sizeof(*file), GFP_KERNEL);
 	if (!file)
 		goto out_unlock;
 
 	memset(file, 0, sizeof(*file));
-	file->f_handle = *fh;
+	memcpy(&file->f_handle, &fh, sizeof(file->f_handle));
 	file->f_sema   = MUTEX;
 
 	/* Open the file. Note that this must not sleep for too long, else
@@ -85,7 +87,7 @@
 	 * We have to make sure we have the right credential to open
 	 * the file.
 	 */
-	if ((nfserr = nlmsvc_ops->fopen(rqstp, fh, &file->f_file)) != 0) {
+	if ((nfserr = nlmsvc_ops->fopen(rqstp, &fh, &file->f_file)) != 0) {
 		dprintk("lockd: open failed (nfserr %d)\n", ntohl(nfserr));
 		goto out_free;
 	}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Andrea Arcangeli]

On Thu, Jan 11, 2001 at 07:30:49PM +0100, Trond Myklebust wrote:
> OK. In that case my patch, would just be amended to eliminate the
> redundant comparison as is the case below.

This patch looks fine w.r.t. alignment but given the below seems called
at runtime (not just at mount time) for performance and to save a dozen of bytes
of kernel stack it would probably better to use the nfs_fh structure in
2.2.19pre7 for the in-kernel representation and to define a new structure for
userspace message passing (defined as the nfs_fh in 2.2.19pre6). But at least
now we see _why_ it broke ;)

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Russell King]

Andrea Arcangeli writes:
> This patch looks fine w.r.t. alignment but given the below seems called
> at runtime (not just at mount time) for performance and to save a dozen of bytes
> of kernel stack it would probably better to use the nfs_fh structure in
> 2.2.19pre7 for the in-kernel representation and to define a new structure for
> userspace message passing (defined as the nfs_fh in 2.2.19pre6). But at least
> now we see _why_ it broke ;)

Ok, this ties up 100% with my suggestion number (1), so I'm happy. ;)
   _____
  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        rmk@arm.linux.org.uk      --- ---
  | | | | http://www.arm.linux.org.uk/personal/aboutme.html   /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Trond Myklebust]

>>>>> " " == Andrea Arcangeli <andrea@suse.de> writes:

     > On Thu, Jan 11, 2001 at 07:30:49PM +0100, Trond Myklebust
     > wrote:
    >> OK. In that case my patch, would just be amended to eliminate
    >> the redundant comparison as is the case below.

     > This patch looks fine w.r.t. alignment but given the below
     > seems called at runtime (not just at mount time) for
     > performance and to save a dozen of bytes of kernel stack it
     > would probably better to use the nfs_fh structure in 2.2.19pre7
     > for the in-kernel representation and to define a new structure
     > for userspace message passing (defined as the nfs_fh in
     > 2.2.19pre6). But at least now we see _why_ it broke ;)

I'm not at all convinced that saving us a copy of 66 bytes in NLM is
really worth the effort. It certainly isn't worth the ugliness of
living with arrays of (void *) in order to match the alignment of a
fake pointer. I'd rather we make them integers.
If we really are to change struct nfs_fh, I therefore propose that we
also make struct nfs_fhbase reflect the fact that fb_dentry is a
32-bit cookie. That means that both nfs_fh and knfs_fh would be
integer-aligned.

The following is untested, but should be correct.

Cheers,
  Trond

diff -u --recursive --new-file linux-2.2.18/fs/lockd/svcsubs.c linux-2.2.18-fix_ppc/fs/lockd/svcsubs.c
--- linux-2.2.18/fs/lockd/svcsubs.c	Mon Dec 11 01:49:44 2000
+++ linux-2.2.18-fix_ppc/fs/lockd/svcsubs.c	Thu Jan 11 20:37:37 2001
@@ -44,6 +44,10 @@
  * Note that we open the file O_RDONLY even when creating write locks.
  * This is not quite right, but for now, we assume the client performs
  * the proper R/W checking.
+ *
+ * BEWARE:
+ * The cast to struct knfs_fh in this routine, imposes an alignment
+ * requirement on (struct nfs_fh)->data for some platforms.
  */
 u32
 nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
@@ -63,8 +67,7 @@
 	down(&nlm_file_sema);
 
 	for (file = nlm_files[hash]; file; file = file->f_next) {
-		if (file->f_handle.fh_dcookie == fh->fh_dcookie &&
-		    !memcmp(&file->f_handle, fh, sizeof(*fh)))
+		if (!memcmp(&file->f_handle, fh, sizeof(*fh)))
 			goto found;
 	}
 
diff -u --recursive --new-file linux-2.2.18/fs/nfs/inode.c linux-2.2.18-fix_ppc/fs/nfs/inode.c
--- linux-2.2.18/fs/nfs/inode.c	Mon Dec 11 01:49:44 2000
+++ linux-2.2.18-fix_ppc/fs/nfs/inode.c	Thu Jan 11 21:13:04 2001
@@ -273,9 +273,8 @@
 	struct nfs_server	*server;
 	struct rpc_xprt		*xprt = 0;
 	struct rpc_clnt		*clnt = 0;
-	struct nfs_fh		*root_fh = NULL,
-				*root = &data->root,
-				fh;
+	struct nfs_fh		fh,
+				*root_fh = NULL;
 	struct inode		*root_inode = NULL;
 	unsigned int		authflavor;
 	struct sockaddr_in	srvaddr;
@@ -290,7 +289,6 @@
 		goto failure;
 	}
 
-	memset(&fh, 0, sizeof(fh));
 	if (data->version != NFS_MOUNT_VERSION) {
 		printk(KERN_WARNING "nfs warning: mount version %s than kernel\n",
 			data->version < NFS_MOUNT_VERSION ? "older" : "newer");
@@ -298,12 +296,21 @@
 			data->namlen = 0;
 		if (data->version < 3)
 			data->bsize  = 0;
-		if (data->version < 4) {
+		if (data->version < 4)
 			data->flags &= ~NFS_MOUNT_VER3;
-			root = &fh;
-			root->size = NFS2_FHSIZE;
-			memcpy(root->data, data->old_root.data, NFS2_FHSIZE);
+	}
+
+	memset(&fh, 0, sizeof(fh));
+	if (data->version < 4) {
+		fh.size = NFS2_FHSIZE;
+		memcpy(fh.data, data->old_root.data, NFS2_FHSIZE);
+	} else {
+		fh.size = (data->flags & NFS_MOUNT_VER3) ? data->root.size : NFS2_FHSIZE;
+		if (fh.size > sizeof(fh.data)) {
+			printk(KERN_WARNING "NFS: mount program passes invalid filehandle!\n");
+			goto failure;
 		}
+		memcpy(fh.data, data->root.data, fh.size);
 	}
 
 	/* We now require that the mount process passes the remote address */
@@ -351,19 +358,15 @@
 	if (data->flags & NFS_MOUNT_VER3) {
 #ifdef CONFIG_NFS_V3
 		server->rpc_ops = &nfs_v3_clientops;
-		NFS_SB_FHSIZE(sb) = sizeof(unsigned short) + NFS3_FHSIZE;
+		NFS_SB_FHSIZE(sb) = NFS3_FHSIZE;
 		version = 3;
-		if (data->version < 4) {
-			printk(KERN_NOTICE "NFS: NFSv3 not supported by mount program.\n");
-			goto failure_unlock;
-		}
 #else
 		printk(KERN_NOTICE "NFS: NFSv3 not supported.\n");
 		goto failure_unlock;
 #endif
 	} else {
 		server->rpc_ops = &nfs_v2_clientops;
-		NFS_SB_FHSIZE(sb) = sizeof(unsigned short) + NFS2_FHSIZE;
+		NFS_SB_FHSIZE(sb) = NFS2_FHSIZE;
 		version = 2;
 	}
 
@@ -422,13 +425,14 @@
 	root_fh = nfs_fh_alloc();
 	if (!root_fh)
 		goto out_no_fh;
-	memcpy((u8*)root_fh, (u8*)root, sizeof(*root_fh));
+	memcpy((u8*)root_fh, (u8*)&fh, sizeof(*root_fh));
 
 	/* Did getting the root inode fail? */
-	if ((root->size > NFS_SB_FHSIZE(sb)
-	     || ! (root_inode = nfs_get_root(sb, root)))
+	if ((fh.size > NFS_SB_FHSIZE(sb)
+	     || ! (root_inode = nfs_get_root(sb, &fh)))
 	    && (data->flags & NFS_MOUNT_VER3)) {
 		data->flags &= ~NFS_MOUNT_VER3;
+		fh.size = NFS2_FHSIZE;
 		nfs_fh_free(root_fh);
 		rpciod_down();
 		rpc_shutdown_client(server->client);
@@ -445,7 +449,7 @@
 	sb->u.nfs_sb.s_root = root_fh;
 
 	/* Get some general file system info */
-	if (server->rpc_ops->statfs(server, root, &fsinfo) >= 0) {
+	if (server->rpc_ops->statfs(server, &fh, &fsinfo) >= 0) {
 		if (server->namelen == 0)
 			server->namelen = fsinfo.namelen;
 	} else {
diff -u --recursive --new-file linux-2.2.18/fs/nfsd/nfsfh.c linux-2.2.18-fix_ppc/fs/nfsd/nfsfh.c
--- linux-2.2.18/fs/nfsd/nfsfh.c	Mon Dec 11 01:49:44 2000
+++ linux-2.2.18-fix_ppc/fs/nfsd/nfsfh.c	Thu Jan 11 20:10:09 2001
@@ -690,7 +690,7 @@
 	fhp->fh_handle.fh_dev    = kdev_t_to_u32(parent->d_inode->i_dev);
 	fhp->fh_handle.fh_xdev   = kdev_t_to_u32(exp->ex_dev);
 	fhp->fh_handle.fh_xino   = ino_t_to_u32(exp->ex_ino);
-	fhp->fh_handle.fh_dcookie = (struct dentry *)0xfeebbaca;
+	fhp->fh_handle.fh_dcookie = 0xfeebbaca;
 	if (inode) {
 		fhp->fh_handle.fh_ino = ino_t_to_u32(inode->i_ino);
 		fhp->fh_handle.fh_generation = inode->i_generation;
diff -u --recursive --new-file linux-2.2.18/include/linux/nfs.h linux-2.2.18-fix_ppc/include/linux/nfs.h
--- linux-2.2.18/include/linux/nfs.h	Mon Dec 11 01:49:44 2000
+++ linux-2.2.18-fix_ppc/include/linux/nfs.h	Thu Jan 11 20:55:26 2001
@@ -94,8 +94,8 @@
  */
 #define NFS_MAXFHSIZE		64
 struct nfs_fh {
-	unsigned short		size;
-	unsigned char		data[NFS_MAXFHSIZE];
+	unsigned int		size;
+	unsigned int		data[NFS_MAXFHSIZE / sizeof(unsigned int)];
 };
 
 enum nfs3_stable_how {
diff -u --recursive --new-file linux-2.2.18/include/linux/nfs3.h linux-2.2.18-fix_ppc/include/linux/nfs3.h
--- linux-2.2.18/include/linux/nfs3.h	Mon Dec 11 01:49:44 2000
+++ linux-2.2.18-fix_ppc/include/linux/nfs3.h	Thu Jan 11 20:06:50 2001
@@ -58,6 +58,11 @@
 	NF3BAD  = 8
 };
 
+struct nfs3_fh {
+	unsigned short		size;
+	unsigned char		data[NFS3_FHSIZE];
+};
+
 #define NFS3_VERSION		3
 #define NFS3PROC_NULL		0
 #define NFS3PROC_GETATTR	1
diff -u --recursive --new-file linux-2.2.18/include/linux/nfs_mount.h linux-2.2.18-fix_ppc/include/linux/nfs_mount.h
--- linux-2.2.18/include/linux/nfs_mount.h	Thu Dec 14 12:30:13 2000
+++ linux-2.2.18-fix_ppc/include/linux/nfs_mount.h	Thu Jan 11 21:21:09 2001
@@ -8,9 +8,9 @@
  *
  *  structure passed from user-space to kernel-space during an nfs mount
  */
-#include <linux/nfs.h>
+#include <linux/in.h>
 #include <linux/nfs2.h>
-#include <linux/nfs_fs.h>
+#include <linux/nfs3.h>
 
 /*
  * WARNING!  Do not delete or change the order of these fields.  If
@@ -42,7 +42,7 @@
 	char		hostname[256];		/* 1 */
 	int		namlen;			/* 2 */
 	unsigned int	bsize;			/* 3 */
-	struct nfs_fh	root;			/* 4 */
+	struct nfs3_fh	root;			/* 4 */
 };
 
 /* bits in the flags field */
diff -u --recursive --new-file linux-2.2.18/include/linux/nfsd/nfsfh.h linux-2.2.18-fix_ppc/include/linux/nfsd/nfsfh.h
--- linux-2.2.18/include/linux/nfsd/nfsfh.h	Thu Dec 14 12:30:28 2000
+++ linux-2.2.18-fix_ppc/include/linux/nfsd/nfsfh.h	Thu Jan 11 20:09:31 2001
@@ -30,7 +30,7 @@
  * ino/dev of the exported inode.
  */
 struct nfs_fhbase {
-	struct dentry *	fb_dentry;	/* dentry cookie */
+	__u32		fb_dcookie;	/* dentry cookie */
 	__u32		fb_ino;		/* our inode number */
 	__u32		fb_dirino;	/* dir inode number */
 	__u32		fb_dev;		/* our device */
@@ -45,7 +45,7 @@
 	__u8			fh_cookie[NFS_FH_PADDING];
 };
 
-#define fh_dcookie		fh_base.fb_dentry
+#define fh_dcookie		fh_base.fb_dcookie
 #define fh_ino			fh_base.fb_ino
 #define fh_dirino		fh_base.fb_dirino
 #define fh_dev			fh_base.fb_dev
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Richard Henderson]

On Thu, Jan 11, 2001 at 12:59:24AM +0100, Andrea Arcangeli wrote:
> What I said is that I can write this C code:
> 
> 	int x[2], * p = (int *) (((char *) &x)+1);
> 	main()
> 	{
> 		*p = 0;
> 	}
> 
> This is legal C code.

Err, no.  This is not "legal" by any stretch of the imagination.
This code has undefined behaviour.

As such, it may work, it may sigbus, it may write data at some
address unrelated to "x", or it may start World War III (with
appropriate hardware attached).

We aren't even obliged to allow this to compile.


r~
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Andrea Arcangeli]

On Tue, Jan 23, 2001 at 11:51:15PM -0800, Richard Henderson wrote:
> On Thu, Jan 11, 2001 at 12:59:24AM +0100, Andrea Arcangeli wrote:
> > What I said is that I can write this C code:
> > 
> > 	int x[2], * p = (int *) (((char *) &x)+1);
> > 	main()
> > 	{
> > 		*p = 0;
> > 	}
> > 
> > This is legal C code.
> 
> Err, no.  This is not "legal" by any stretch of the imagination.
> This code has undefined behaviour.

I know this code has undefined behaviour at _runtime_. But I thought
you were obliged to allow it to compile. That was my only point.

> We aren't even obliged to allow this to compile.

I'd love if you could forbid it to compile.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Richard Henderson]

On Wed, Jan 24, 2001 at 10:02:40AM +0100, Andrea Arcangeli wrote:
> I'd love if you could forbid it to compile.

Problem is that there's stuff like this all over the place.  Plus,
just because something is undefined by the standard doesn't mean
it's not useful -- it's not possible to write either a kernel or
libc without breaking some rules.

So any useful compile-time restrictions we could make is if the
static expression was not computable (perhaps due to being a word
addressed machine), or if we could somehow magically divine that
the store would in fact go wrong at runtime.


r~
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Thomas Pornin]

In article <20010124100240.A4526@athlon.random> you write:
> I know this code has undefined behaviour at _runtime_. But I thought
> you were obliged to allow it to compile. That was my only point.

There is no distinction between compilation and runtime in the standard.
Actually, C could be interpreted, or a very smart compiler could also
think real hard and replace the whole program by an equivalent printf().

Besides, a standard (C99) compiler will reject the 'main' definition.
At least, the return type cannot be implicit anymore.


	--Thomas Pornin
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Andrea Arcangeli]

On Wed, Jan 24, 2001 at 01:51:49AM -0800, Richard Henderson wrote:
> On Wed, Jan 24, 2001 at 10:02:40AM +0100, Andrea Arcangeli wrote:
> > I'd love if you could forbid it to compile.
> 
> Problem is that there's stuff like this all over the place.  Plus,

That's why I thought you were required to make it to compile. For example
you don't know if there's another object that will cast the int pointer back to
char pointer before dereferencing. That would get a defined runtime behaviour
on all archs.

And yes, I see above I'm not allowed to say "runtime" nor "defined behaviour"
according to the standard, but that's how thing works in practice.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Richard Henderson]

On Wed, Jan 24, 2001 at 01:21:44PM +0100, Andrea Arcangeli wrote:
> For example you don't know if there's another object that will cast
> the int pointer back to char pointer before dereferencing. That would
> get a defined runtime behaviour on all archs.

No.  The representation of "int*" and "char*" needn't be the same.
And isn't on old word addressed machines.


r~
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: Compatibility issue with 2.2.19pre7

[Jesse Pollard]

Andrea Arcangeli <andrea@suse.de>:
> On Tue, Jan 23, 2001 at 11:51:15PM -0800, Richard Henderson wrote:
> > On Thu, Jan 11, 2001 at 12:59:24AM +0100, Andrea Arcangeli wrote:
> > > What I said is that I can write this C code:
> > > 
> > > 	int x[2], * p = (int *) (((char *) &x)+1);
> > > 	main()
> > > 	{
> > > 		*p = 0;
> > > 	}
> > > 
> > > This is legal C code.
> > 
> > Err, no.  This is not "legal" by any stretch of the imagination.
> > This code has undefined behaviour.
> 
> I know this code has undefined behaviour at _runtime_. But I thought
> you were obliged to allow it to compile. That was my only point.
> 
> > We aren't even obliged to allow this to compile.
> 
> I'd love if you could forbid it to compile.

It would have to be implementation dependant/architecture dependant-
The only way to know is to realize that *p initializer value is invalid
is to know:

1. integer addresses MUST be aligned (say 4 or 8 byte boundaries)
2. the computation of (int *) (((char *) &x)+1) is a constant at
   compile time, and not link time (some linkers can do address offset
   calculations if "x" happens to be an external - ie. x +/- offset and
   offset is a constant)

Other systems would have to allow it to compile (and link). It still has
undefined behaviour, and is non-portable.

-------------------------------------------------------------------------
Jesse I Pollard, II
Email: pollard@navo.hpc.mil

Any opinions expressed are solely my own.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/