Use of ptrdiff_t in
- if (!access_ok(VERIFY_WRITE, u_tmp->rx_buf, u_tmp->len))
+ if (!access_ok(VERIFY_WRITE, (u8 __user *)
+ (ptrdiff_t) u_tmp->rx_buf,
+ u_tmp->len))
is wrong; for one thing, it's a bad C (it's what uintptr_t is for; in general
we are not even promised that ptrdiff_t is large enough to hold a pointer,
just enough to hold a difference between two pointers within the same object).
For another, it confuses the fsck out of sparse.
Use unsigned long or uintptr_t instead. There are several places misusing
ptrdiff_t (one - in a very odd way; WTF did that cast to __user ptrdiff_t
in ntfs expect to happen, anyway?) Fixed...
Signed-off-by: Al Viro <[email protected]>
---
diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index 72b0393..1e6d7a9 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -391,7 +391,7 @@ static int close_getadapter_fib(struct aac_dev * dev, void __user *arg)
/*
* Extract the fibctx from the input parameters
*/
- if (fibctx->unique == (u32)(ptrdiff_t)arg) /* We found a winner */
+ if (fibctx->unique == (u32)(uintptr_t)arg) /* We found a winner */
break;
entry = entry->next;
fibctx = NULL;
@@ -590,7 +590,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
}
addr = (u64)upsg->sg[i].addr[0];
addr += ((u64)upsg->sg[i].addr[1]) << 32;
- sg_user[i] = (void __user *)(ptrdiff_t)addr;
+ sg_user[i] = (void __user *)(uintptr_t)addr;
sg_list[i] = p; // save so we can clean up later
sg_indx = i;
@@ -633,7 +633,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
rcode = -ENOMEM;
goto cleanup;
}
- sg_user[i] = (void __user *)(ptrdiff_t)usg->sg[i].addr;
+ sg_user[i] = (void __user *)(uintptr_t)usg->sg[i].addr;
sg_list[i] = p; // save so we can clean up later
sg_indx = i;
@@ -664,7 +664,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
if (actual_fibsize64 == fibsize) {
struct user_sgmap64* usg = (struct user_sgmap64 *)upsg;
for (i = 0; i < upsg->count; i++) {
- u64 addr;
+ uintptr_t addr;
void* p;
/* Does this really need to be GFP_DMA? */
p = kmalloc(usg->sg[i].count,GFP_KERNEL|__GFP_DMA);
@@ -676,7 +676,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
}
addr = (u64)usg->sg[i].addr[0];
addr += ((u64)usg->sg[i].addr[1]) << 32;
- sg_user[i] = (void __user *)(ptrdiff_t)addr;
+ sg_user[i] = (void __user *)addr;
sg_list[i] = p; // save so we can clean up later
sg_indx = i;
@@ -704,7 +704,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
rcode = -ENOMEM;
goto cleanup;
}
- sg_user[i] = (void __user *)(ptrdiff_t)upsg->sg[i].addr;
+ sg_user[i] = (void __user *)(uintptr_t)upsg->sg[i].addr;
sg_list[i] = p; // save so we can clean up later
sg_indx = i;
diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
index 3009ad8..8736813 100644
--- a/drivers/scsi/aacraid/comminit.c
+++ b/drivers/scsi/aacraid/comminit.c
@@ -110,7 +110,7 @@ static int aac_alloc_comm(struct aac_dev *dev, void **commaddr, unsigned long co
/*
* Align the beginning of Headers to commalign
*/
- align = (commalign - ((ptrdiff_t)(base) & (commalign - 1)));
+ align = (commalign - ((uintptr_t)(base) & (commalign - 1)));
base = base + align;
phys = phys + align;
/*
diff --git a/drivers/scsi/aacraid/dpcsup.c b/drivers/scsi/aacraid/dpcsup.c
index fcd25f7..e6032ff 100644
--- a/drivers/scsi/aacraid/dpcsup.c
+++ b/drivers/scsi/aacraid/dpcsup.c
@@ -254,7 +254,7 @@ unsigned int aac_intr_normal(struct aac_dev * dev, u32 Index)
kfree (fib);
return 1;
}
- memcpy(hw_fib, (struct hw_fib *)(((ptrdiff_t)(dev->regs.sa)) +
+ memcpy(hw_fib, (struct hw_fib *)(((uintptr_t)(dev->regs.sa)) +
(index & ~0x00000002L)), sizeof(struct hw_fib));
INIT_LIST_HEAD(&fib->fiblink);
fib->type = FSAFS_NTC_FIB_CONTEXT;
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index c55459c..b3518ca 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -184,14 +184,14 @@ static int spidev_message(struct spidev_data *spidev,
if (u_tmp->rx_buf) {
k_tmp->rx_buf = buf;
if (!access_ok(VERIFY_WRITE, (u8 __user *)
- (ptrdiff_t) u_tmp->rx_buf,
+ (uintptr_t) u_tmp->rx_buf,
u_tmp->len))
goto done;
}
if (u_tmp->tx_buf) {
k_tmp->tx_buf = buf;
if (copy_from_user(buf, (const u8 __user *)
- (ptrdiff_t) u_tmp->tx_buf,
+ (uintptr_t) u_tmp->tx_buf,
u_tmp->len))
goto done;
}
@@ -224,7 +224,7 @@ static int spidev_message(struct spidev_data *spidev,
for (n = n_xfers, u_tmp = u_xfers; n; n--, u_tmp++) {
if (u_tmp->rx_buf) {
if (__copy_to_user((u8 __user *)
- (ptrdiff_t) u_tmp->rx_buf, buf,
+ (uintptr_t) u_tmp->rx_buf, buf,
u_tmp->len)) {
status = -EFAULT;
goto done;
diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
index ffcc504..a9103ce 100644
--- a/fs/ntfs/file.c
+++ b/fs/ntfs/file.c
@@ -362,7 +362,7 @@ static inline void ntfs_fault_in_pages_readable(const char __user *uaddr,
volatile char c;
/* Set @end to the first byte outside the last page we care about. */
- end = (const char __user*)PAGE_ALIGN((ptrdiff_t __user)uaddr + bytes);
+ end = (const char __user*)PAGE_ALIGN((uintptr_t)uaddr + bytes);
while (!__get_user(c, uaddr) && (uaddr += PAGE_SIZE, uaddr < end))
;
diff --git a/include/linux/types.h b/include/linux/types.h
index 0351bf2..efdec19 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -40,6 +40,8 @@ typedef __kernel_gid32_t gid_t;
typedef __kernel_uid16_t uid16_t;
typedef __kernel_gid16_t gid16_t;
+typedef unsigned long uintptr_t;
+
#ifdef CONFIG_UID16
/* This is defined by include/asm-{arch}/posix_types.h */
typedef __kernel_old_uid_t old_uid_t;
On Sunday 19 August 2007, Al Viro wrote:
> is wrong; for one thing, it's a bad C (it's what uintptr_t is for; in general
> we are not even promised that ptrdiff_t is large enough to hold a pointer,
ISTR we don't *have* a uintptr_t on all architectures, or that would
be the appropriate thing to use in these 32/64 bit ABI scenarios.
> Use unsigned long or uintptr_t instead.
I suspect you mean "unsigned long long"...
- Dave
On 19 Aug 2007, at 23:55, Al Viro wrote:
> Use of ptrdiff_t in
>
> - if (!access_ok(VERIFY_WRITE, u_tmp->rx_buf,
> u_tmp->len))
> + if (!access_ok(VERIFY_WRITE, (u8 __user *)
> + (ptrdiff_t) u_tmp-
> >rx_buf,
> + u_tmp->len))
>
> is wrong; for one thing, it's a bad C (it's what uintptr_t is for;
> in general
> we are not even promised that ptrdiff_t is large enough to hold a
> pointer,
> just enough to hold a difference between two pointers within the
> same object).
> For another, it confuses the fsck out of sparse.
>
> Use unsigned long or uintptr_t instead. There are several places
> misusing
> ptrdiff_t (one - in a very odd way; WTF did that cast to __user
> ptrdiff_t
> in ntfs expect to happen, anyway?) Fixed...
My current NTFS code has this already fixed. I used unsigned long
instead of uintptr_t though... Feel free to apply this though as I
have no idea when I will have time/permission to push an update
upstream...
And what the cast was doing I can't remember. I may well have just
copied it from the VFS or I was perhaps trying to silence a warning
and this happened to work... But yes I noticed that more recent
versions of sparse complained about it and fixed it with an unsigned
long cast.
Best regards,
Anton
>
> Signed-off-by: Al Viro <[email protected]>
> ---
> diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/
> commctrl.c
> index 72b0393..1e6d7a9 100644
> --- a/drivers/scsi/aacraid/commctrl.c
> +++ b/drivers/scsi/aacraid/commctrl.c
> @@ -391,7 +391,7 @@ static int close_getadapter_fib(struct aac_dev
> * dev, void __user *arg)
> /*
> * Extract the fibctx from the input parameters
> */
> - if (fibctx->unique == (u32)(ptrdiff_t)arg) /* We found a winner */
> + if (fibctx->unique == (u32)(uintptr_t)arg) /* We found a winner */
> break;
> entry = entry->next;
> fibctx = NULL;
> @@ -590,7 +590,7 @@ static int aac_send_raw_srb(struct aac_dev*
> dev, void __user * arg)
> }
> addr = (u64)upsg->sg[i].addr[0];
> addr += ((u64)upsg->sg[i].addr[1]) << 32;
> - sg_user[i] = (void __user *)(ptrdiff_t)addr;
> + sg_user[i] = (void __user *)(uintptr_t)addr;
> sg_list[i] = p; // save so we can clean up later
> sg_indx = i;
>
> @@ -633,7 +633,7 @@ static int aac_send_raw_srb(struct aac_dev*
> dev, void __user * arg)
> rcode = -ENOMEM;
> goto cleanup;
> }
> - sg_user[i] = (void __user *)(ptrdiff_t)usg->sg[i].addr;
> + sg_user[i] = (void __user *)(uintptr_t)usg->sg[i].addr;
> sg_list[i] = p; // save so we can clean up later
> sg_indx = i;
>
> @@ -664,7 +664,7 @@ static int aac_send_raw_srb(struct aac_dev*
> dev, void __user * arg)
> if (actual_fibsize64 == fibsize) {
> struct user_sgmap64* usg = (struct user_sgmap64 *)upsg;
> for (i = 0; i < upsg->count; i++) {
> - u64 addr;
> + uintptr_t addr;
> void* p;
> /* Does this really need to be GFP_DMA? */
> p = kmalloc(usg->sg[i].count,GFP_KERNEL|__GFP_DMA);
> @@ -676,7 +676,7 @@ static int aac_send_raw_srb(struct aac_dev*
> dev, void __user * arg)
> }
> addr = (u64)usg->sg[i].addr[0];
> addr += ((u64)usg->sg[i].addr[1]) << 32;
> - sg_user[i] = (void __user *)(ptrdiff_t)addr;
> + sg_user[i] = (void __user *)addr;
> sg_list[i] = p; // save so we can clean up later
> sg_indx = i;
>
> @@ -704,7 +704,7 @@ static int aac_send_raw_srb(struct aac_dev*
> dev, void __user * arg)
> rcode = -ENOMEM;
> goto cleanup;
> }
> - sg_user[i] = (void __user *)(ptrdiff_t)upsg->sg[i].addr;
> + sg_user[i] = (void __user *)(uintptr_t)upsg->sg[i].addr;
> sg_list[i] = p; // save so we can clean up later
> sg_indx = i;
>
> diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/
> comminit.c
> index 3009ad8..8736813 100644
> --- a/drivers/scsi/aacraid/comminit.c
> +++ b/drivers/scsi/aacraid/comminit.c
> @@ -110,7 +110,7 @@ static int aac_alloc_comm(struct aac_dev *dev,
> void **commaddr, unsigned long co
> /*
> * Align the beginning of Headers to commalign
> */
> - align = (commalign - ((ptrdiff_t)(base) & (commalign - 1)));
> + align = (commalign - ((uintptr_t)(base) & (commalign - 1)));
> base = base + align;
> phys = phys + align;
> /*
> diff --git a/drivers/scsi/aacraid/dpcsup.c b/drivers/scsi/aacraid/
> dpcsup.c
> index fcd25f7..e6032ff 100644
> --- a/drivers/scsi/aacraid/dpcsup.c
> +++ b/drivers/scsi/aacraid/dpcsup.c
> @@ -254,7 +254,7 @@ unsigned int aac_intr_normal(struct aac_dev *
> dev, u32 Index)
> kfree (fib);
> return 1;
> }
> - memcpy(hw_fib, (struct hw_fib *)(((ptrdiff_t)(dev->regs.sa)) +
> + memcpy(hw_fib, (struct hw_fib *)(((uintptr_t)(dev->regs.sa)) +
> (index & ~0x00000002L)), sizeof(struct hw_fib));
> INIT_LIST_HEAD(&fib->fiblink);
> fib->type = FSAFS_NTC_FIB_CONTEXT;
> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> index c55459c..b3518ca 100644
> --- a/drivers/spi/spidev.c
> +++ b/drivers/spi/spidev.c
> @@ -184,14 +184,14 @@ static int spidev_message(struct spidev_data
> *spidev,
> if (u_tmp->rx_buf) {
> k_tmp->rx_buf = buf;
> if (!access_ok(VERIFY_WRITE, (u8 __user *)
> - (ptrdiff_t) u_tmp->rx_buf,
> + (uintptr_t) u_tmp->rx_buf,
> u_tmp->len))
> goto done;
> }
> if (u_tmp->tx_buf) {
> k_tmp->tx_buf = buf;
> if (copy_from_user(buf, (const u8 __user *)
> - (ptrdiff_t) u_tmp->tx_buf,
> + (uintptr_t) u_tmp->tx_buf,
> u_tmp->len))
> goto done;
> }
> @@ -224,7 +224,7 @@ static int spidev_message(struct spidev_data
> *spidev,
> for (n = n_xfers, u_tmp = u_xfers; n; n--, u_tmp++) {
> if (u_tmp->rx_buf) {
> if (__copy_to_user((u8 __user *)
> - (ptrdiff_t) u_tmp->rx_buf, buf,
> + (uintptr_t) u_tmp->rx_buf, buf,
> u_tmp->len)) {
> status = -EFAULT;
> goto done;
> diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
> index ffcc504..a9103ce 100644
> --- a/fs/ntfs/file.c
> +++ b/fs/ntfs/file.c
> @@ -362,7 +362,7 @@ static inline void ntfs_fault_in_pages_readable
> (const char __user *uaddr,
> volatile char c;
>
> /* Set @end to the first byte outside the last page we care
> about. */
> - end = (const char __user*)PAGE_ALIGN((ptrdiff_t __user)uaddr +
> bytes);
> + end = (const char __user*)PAGE_ALIGN((uintptr_t)uaddr + bytes);
>
> while (!__get_user(c, uaddr) && (uaddr += PAGE_SIZE, uaddr < end))
> ;
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 0351bf2..efdec19 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -40,6 +40,8 @@ typedef __kernel_gid32_t gid_t;
> typedef __kernel_uid16_t uid16_t;
> typedef __kernel_gid16_t gid16_t;
>
> +typedef unsigned long uintptr_t;
> +
> #ifdef CONFIG_UID16
> /* This is defined by include/asm-{arch}/posix_types.h */
> typedef __kernel_old_uid_t old_uid_t;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-
> kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Best regards,
Anton
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/
Hi,
On 20 Aug 2007, at 01:19, David Brownell wrote:
> On Sunday 19 August 2007, Al Viro wrote:
>> is wrong; for one thing, it's a bad C (it's what uintptr_t is for;
>> in general
>> we are not even promised that ptrdiff_t is large enough to hold a
>> pointer,
>
> ISTR we don't *have* a uintptr_t on all architectures, or that would
> be the appropriate thing to use in these 32/64 bit ABI scenarios.
>
>
>> Use unsigned long or uintptr_t instead.
>
> I suspect you mean "unsigned long long"...
No he doesn't. "unsigned long" is guaranteed to be large enough to
hold a pointer (at least on Linux anyway).
On a 32-bit arch "unsigned long" is 32-bit and pointers are 32-bit.
On a 64-bit archi "unsigned long" is 64-bit and pointers are 64-bit.
Best regards,
Anton
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/
On Mon, Aug 20, 2007 at 01:27:13AM +0100, Anton Altaparmakov wrote:
> And what the cast was doing I can't remember. I may well have just
> copied it from the VFS or I was perhaps trying to silence a warning
> and this happened to work...
... due to sparse bug (it miscalculated address space of pointed to,
picking top-level qualifiers for some reason; __user pointer to char
and pointer to __user char gave the same result and so did __user long).
That cast still made no sense...
On Sunday 19 August 2007, Anton Altaparmakov wrote:
> >
> > ISTR we don't *have* a uintptr_t on all architectures, or that would
> > be the appropriate thing to use in these 32/64 bit ABI scenarios.
> >
> >
> >> Use unsigned long or uintptr_t instead.
> >
> > I suspect you mean "unsigned long long"...
>
> No he doesn't. ?"unsigned long" is guaranteed to be large enough to ?
> hold a pointer (at least on Linux anyway).
And yet when I used that, I got compiler warnings on some systems.
ISTR that was the first solution I tried, but GCC really wanted to
issue warnings. Either about casting 64-bit to pointer, or about
casting it to "unsigned long", either way lost precision.
> On a 32-bit arch "unsigned long" is 32-bit and pointers are 32-bit.
>
> On a 64-bit archi "unsigned long" is 64-bit and pointers are 64-bit.
So with 32 bit userspace "unsigned long long" is the type to use
when talking to a 64-bit kernel; and with pure 64-bit code, it's
enough to write "unsigned long".
I'm fairly sure that's the root cause of the pain I recall here;
but I'd have to run experiments again to verify that.
On Mon, Aug 20, 2007 at 01:29:21AM +0100, Anton Altaparmakov wrote:
> Hi,
>
> On 20 Aug 2007, at 01:19, David Brownell wrote:
> >On Sunday 19 August 2007, Al Viro wrote:
> >>is wrong; for one thing, it's a bad C (it's what uintptr_t is for;
> >>in general
> >>we are not even promised that ptrdiff_t is large enough to hold a
> >>pointer,
> >
> >ISTR we don't *have* a uintptr_t on all architectures, or that would
> >be the appropriate thing to use in these 32/64 bit ABI scenarios.
> >
> >
> >>Use unsigned long or uintptr_t instead.
> >
> >I suspect you mean "unsigned long long"...
>
> No he doesn't. "unsigned long" is guaranteed to be large enough to
> hold a pointer (at least on Linux anyway).
>
> On a 32-bit arch "unsigned long" is 32-bit and pointers are 32-bit.
... while unsigned long long is 64bit, which is definitely not what
one wants. For sparse it's "unsigned long is special".
FWIW, this patch puts it in linux/types.h as unsigned long. Eventually
we might want to switch explicit casts to/from unsigned long in such contexts
to uintptr_t, but for now we can't start complaining about unsigned long -
too many places are using it. I'll see what can be done to get sane
assistance from sparse in that kind of work...
On Sunday 19 August 2007, Al Viro wrote:
> On Mon, Aug 20, 2007 at 01:27:13AM +0100, Anton Altaparmakov wrote:
>
> > And what the cast was doing I can't remember. I may well have just
> > copied it from the VFS or I was perhaps trying to silence a warning
> > and this happened to work...
>
> ... due to sparse bug (it miscalculated address space of pointed to,
> picking top-level qualifiers for some reason; __user pointer to char
> and pointer to __user char gave the same result and so did __user long).
> That cast still made no sense...
I could believe that all came from sparse bugs. Fixed now?
On Sun, 19 Aug 2007, David Brownell wrote:
> On Sunday 19 August 2007, Anton Altaparmakov wrote:
> > >
> > > ISTR we don't *have* a uintptr_t on all architectures, or that would
> > > be the appropriate thing to use in these 32/64 bit ABI scenarios.
> > >
> > >
> > >> Use unsigned long or uintptr_t instead.
> > >
> > > I suspect you mean "unsigned long long"...
> >
> > No he doesn't. "unsigned long" is guaranteed to be large enough to
> > hold a pointer (at least on Linux anyway).
Yup, sizeof(long) >= sizeof(void *) should always be true in Linux C.
I bet a lot of code out there depends on this.
BTW, just curious to know, but which (if any) are the platforms that have
sizeof(long) > sizeof(void *)? [i.e. greater-than but not equal?]
The reason I ask is that gcc will also complain (understandably so) with
"warning: cast from pointer to integer of different size" i.e. even if
it's a conversion from smaller size to greater size, and not really a
case of truncation. Therefore, I wonder if the stricter assertion:
sizeof(long) == sizeof(void *) holds true for Linux C, actually.
> And yet when I used that, I got compiler warnings on some systems.
>
> ISTR that was the first solution I tried, but GCC really wanted to
> issue warnings. Either about casting 64-bit to pointer, or about
> casting it to "unsigned long", either way lost precision.
Hmm, if you could fish out those testcases ...
> > On a 32-bit arch "unsigned long" is 32-bit and pointers are 32-bit.
> >
> > On a 64-bit archi "unsigned long" is 64-bit and pointers are 64-bit.
>
> So with 32 bit userspace "unsigned long long" is the type to use
> when talking to a 64-bit kernel; and with pure 64-bit code, it's
> enough to write "unsigned long".
>
> I'm fairly sure that's the root cause of the pain I recall here;
> but I'd have to run experiments again to verify that.
I suspect the root cause of the pain was that you used "int" or "long"
to talk between kernel and userspace in the first place. You shouldn't,
we have __u32 / __u64 / etc for that.
Satyam
On Sunday 19 August 2007, Satyam Sharma wrote:
> > > On a 32-bit arch "unsigned long" is 32-bit and pointers are 32-bit.
> > >
> > > On a 64-bit archi "unsigned long" is 64-bit and pointers are 64-bit.
> >
> > So with 32 bit userspace "unsigned long long" is the type to use
> > when talking to a 64-bit kernel; and with pure 64-bit code, it's
> > enough to write "unsigned long".
> >
> > I'm fairly sure that's the root cause of the pain I recall here;
> > but I'd have to run experiments again to verify that. ?
>
> I suspect the root cause of the pain was that you used "int" or "long"
> to talk between kernel and userspace in the first place. You shouldn't,
> we have __u32 / __u64 / etc for that.
Nope; the relevant code was always with "__u64". The issue was
warning when turning that into a __user pointer.
> The reason I ask is that gcc will also complain (understandably so) with
> "warning: cast from pointer to integer of different size" i.e. even if
> it's a conversion from smaller size to greater size, and not really a
> case of truncation.
ISTR the warning was the other way around: about "cast from integer
to pointer of a different size". The __u64 came from userspace and
the kernel pointer was only 32 bits. Not really truncation, but GCC
could not know that directly ... ergo the extra non-pointer cast.
On Sun, Aug 19, 2007 at 08:26:24PM -0700, David Brownell wrote:
> ISTR the warning was the other way around: about "cast from integer
> to pointer of a different size". The __u64 came from userspace and
> the kernel pointer was only 32 bits. Not really truncation, but GCC
> could not know that directly ... ergo the extra non-pointer cast.
And? Cast to integer type with the size equal to that of pointer.
unsigned long is just that on all supported targets.
More interesting question is whether you want an error returned when
pointers are 32bit and value doesn't fit into that...
On Sunday 19 August 2007, Al Viro wrote:
> On Sun, Aug 19, 2007 at 08:26:24PM -0700, David Brownell wrote:
>
> > ISTR the warning was the other way around: about "cast from integer
> > to pointer of a different size". The __u64 came from userspace and
> > the kernel pointer was only 32 bits. Not really truncation, but GCC
> > could not know that directly ... ergo the extra non-pointer cast.
>
> And? Cast to integer type with the size equal to that of pointer.
> unsigned long is just that on all supported targets.
Some tool kept warning about that. Presumably then-current sparse.
I've certainly heard the conventional "unsigned long fits pointers"
wisdom, but tools disagreed. (Does ANSI C guarantee that? I'd think
not, or uintptr_t would not be needed.)
And ptrdiff_t was the closest relevant data type that passed both
gcc and sparse, since uintptr_t didn't previously exist everywhere.
> More interesting question is whether you want an error returned when
> pointers are 32bit and value doesn't fit into that...
Either access_ok() or copy_from_user() reports an error if the
pointer part of that u64 (N LSBs) is bad.
As a general policy, I think the other part is undefined and
irrelevant to the kernel ... it's a kind of explicit padding,
and padding isn't valdated. (At most it's zeroed to prevent
a covert channel, but that's not relevent here.)
- Dave
On Sunday 19 August 2007, Al Viro wrote:
> Use unsigned long or uintptr_t instead. ?There are several places misusing
> ptrdiff_t (one - in a very odd way; WTF did that cast to __user ptrdiff_t
> in ntfs expect to happen, anyway?) ? Fixed...
>
> Signed-off-by: Al Viro <[email protected]>
By the way: ACK. Glad there's now always a "uintptr_t".