2.6.17-rc1 core_sys_select incompatible pointer types

2.6.17-rc1 core_sys_select incompatible pointer types

[Keith Owens]

2.6.17-rc1, ia64, gcc 3.3.3

fs/select.c: In function `core_sys_select':
fs/select.c:339: warning: assignment from incompatible pointer type
fs/select.c:376: warning: comparison of distinct pointer types lacks a cast

Re: 2.6.17-rc1 core_sys_select incompatible pointer types

[Mitchell Blank Jr]

Keith Owens wrote:
> 2.6.17-rc1, ia64, gcc 3.3.3
> 
> fs/select.c: In function `core_sys_select':
> fs/select.c:339: warning: assignment from incompatible pointer type
> fs/select.c:376: warning: comparison of distinct pointer types lacks a cast

I posted a patch to fix this and another problem with the recent select
changes a couple days ago.

Original version, with description:
  http://lkml.org/lkml/2006/3/31/308
Slightly updated:
  http://lkml.org/lkml/2006/3/31/316

I'm hoping that Andrew picked it up.  I'm waiting for the next -mm to see
if I need to agitate more :-)

-Mitch

Re: 2.6.17-rc1 core_sys_select incompatible pointer types

[Andrew Morton]

Mitchell Blank Jr <mitch@sfgoth.com> wrote:
>
>  Keith Owens wrote:
>  > 2.6.17-rc1, ia64, gcc 3.3.3
>  > 
>  > fs/select.c: In function `core_sys_select':
>  > fs/select.c:339: warning: assignment from incompatible pointer type
>  > fs/select.c:376: warning: comparison of distinct pointer types lacks a cast
> 
>  I posted a patch to fix this and another problem with the recent select
>  changes a couple days ago.
> 
>  Original version, with description:
>    http://lkml.org/lkml/2006/3/31/308
>  Slightly updated:
>    http://lkml.org/lkml/2006/3/31/316
> 
>  I'm hoping that Andrew picked it up.

Nope.  I queued up the below.  If anything additional is needed, please
resend.


diff -puN fs/select.c~select-warning-fixes fs/select.c
--- devel/fs/select.c~select-warning-fixes	2006-04-01 22:27:14.000000000 -0800
+++ devel-akpm/fs/select.c	2006-04-01 22:28:50.000000000 -0800
@@ -310,7 +310,7 @@ static int core_sys_select(int n, fd_set
 			   fd_set __user *exp, s64 *timeout)
 {
 	fd_set_bits fds;
-	char *bits;
+	void *bits;
 	int ret, size, max_fdset;
 	struct fdtable *fdt;
 	/* Allocate small arguments on the stack to save memory and be faster */
@@ -341,12 +341,12 @@ static int core_sys_select(int n, fd_set
 		bits = kmalloc(6 * size, GFP_KERNEL);
 	if (!bits)
 		goto out_nofds;
-	fds.in      = (unsigned long *)  bits;
-	fds.out     = (unsigned long *) (bits +   size);
-	fds.ex      = (unsigned long *) (bits + 2*size);
-	fds.res_in  = (unsigned long *) (bits + 3*size);
-	fds.res_out = (unsigned long *) (bits + 4*size);
-	fds.res_ex  = (unsigned long *) (bits + 5*size);
+	fds.in      = bits;
+	fds.out     = bits +   size;
+	fds.ex      = bits + 2*size;
+	fds.res_in  = bits + 3*size;
+	fds.res_out = bits + 4*size;
+	fds.res_ex  = bits + 5*size;
 
 	if ((ret = get_fd_set(n, inp, fds.in)) ||
 	    (ret = get_fd_set(n, outp, fds.out)) ||
_

Re: 2.6.17-rc1 core_sys_select incompatible pointer types

[Keith Owens]

Andrew Morton (on Mon, 3 Apr 2006 02:09:16 -0700) wrote:
>Mitchell Blank Jr <mitch@sfgoth.com> wrote:
>>  I posted a patch to fix this and another problem with the recent select
>>  changes a couple days ago.
>> 
>>  Original version, with description:
>>    http://lkml.org/lkml/2006/3/31/308
>>  Slightly updated:
>>    http://lkml.org/lkml/2006/3/31/316
>> 
>>  I'm hoping that Andrew picked it up.
>
>Nope.  I queued up the below.  If anything additional is needed, please
>resend.
>
>
>diff -puN fs/select.c~select-warning-fixes fs/select.c
>--- devel/fs/select.c~select-warning-fixes	2006-04-01 22:27:14.000000000 -0800
>+++ devel-akpm/fs/select.c	2006-04-01 22:28:50.000000000 -0800
>@@ -310,7 +310,7 @@ static int core_sys_select(int n, fd_set
> 			   fd_set __user *exp, s64 *timeout)
> {
> 	fd_set_bits fds;
>-	char *bits;
>+	void *bits;
> 	int ret, size, max_fdset;
> 	struct fdtable *fdt;
> 	/* Allocate small arguments on the stack to save memory and be faster */
>@@ -341,12 +341,12 @@ static int core_sys_select(int n, fd_set
> 		bits = kmalloc(6 * size, GFP_KERNEL);
> 	if (!bits)
> 		goto out_nofds;
>-	fds.in      = (unsigned long *)  bits;
>-	fds.out     = (unsigned long *) (bits +   size);
>-	fds.ex      = (unsigned long *) (bits + 2*size);
>-	fds.res_in  = (unsigned long *) (bits + 3*size);
>-	fds.res_out = (unsigned long *) (bits + 4*size);
>-	fds.res_ex  = (unsigned long *) (bits + 5*size);
>+	fds.in      = bits;
>+	fds.out     = bits +   size;
>+	fds.ex      = bits + 2*size;
>+	fds.res_in  = bits + 3*size;
>+	fds.res_out = bits + 4*size;
>+	fds.res_ex  = bits + 5*size;
> 
> 	if ((ret = get_fd_set(n, inp, fds.in)) ||
> 	    (ret = get_fd_set(n, outp, fds.out)) ||

When did arithmetic on void pointers become acceptable?  I know that it
is a gcc extension but AFAIK its use is discouraged.  Or am I in the
wrong parallel universe again?

Re: 2.6.17-rc1 core_sys_select incompatible pointer types

[Andrew Morton]

Keith Owens <kaos@sgi.com> wrote:
>
>  When did arithmetic on void pointers become acceptable?  I know that it
>  is a gcc extension but AFAIK its use is discouraged.  Or am I in the
>  wrong parallel universe again?

Yes, it's a gccism, but we actually use it rather a lot here and there. 
It's quite useful for avoiding casting.

Re: 2.6.17-rc1 core_sys_select incompatible pointer types

[Mitchell Blank Jr]

Andrew Morton wrote:
> Nope.  I queued up the below.  If anything additional is needed, please
> resend.

Could you at least apply this bit on top?  I actually combed select.c
yesterday and have a bunch of changes, but I can send those to you as a
patch series later.  This one I'd like to land now since it fixes a bug
introduced within the last week (even though the bug isn't currently
triggerable)

The code is (very slightly) rearranged; should be a little more readable
and quicker for the fastpath.

Subject: [SELECT] don't overflow if (SELECT_STACK_ALLOC % sizeof(long) != 0)

If SELECT_STACK_ALLOC is not a multiple of sizeof(long) then stack_fds[]
would be shorter than SELECT_STACK_ALLOC bytes and could overflow later
in the function.  Fixed by simply rearranging the test later to work on
sizeof(stack_fds)  Currently SELECT_STACK_ALLOC is 256 so this doesn't happen,
but it's nasty to have things like this hidden in the code.  What if later
someone decides to change SELECT_STACK_ALLOC to 300?

Signed-off-by: Mitchell Blank Jr <mitch@sfgoth.com>

--- linux-2.6/fs/select.c-AKPM	2006-04-03 02:25:34.000000000 -0700
+++ linux-2.6/fs/select.c	2006-04-03 02:32:53.000000000 -0700
@@ -311,7 +311,8 @@
 {
 	fd_set_bits fds;
 	void *bits;
-	int ret, size, max_fdset;
+	int ret, max_fdset;
+	unsigned int size;
 	struct fdtable *fdt;
 	/* Allocate small arguments on the stack to save memory and be faster */
 	long stack_fds[SELECT_STACK_ALLOC/sizeof(long)];
@@ -333,14 +334,15 @@
 	 * since we used fdset we need to allocate memory in units of
 	 * long-words. 
 	 */
-	ret = -ENOMEM;
 	size = FDS_BYTES(n);
-	if (6*size < SELECT_STACK_ALLOC)
-		bits = stack_fds;
-	else
+	bits = stack_fds;
+	if (size > sizeof(stack_fds) / 6) {
+		/* Not enough space in on-stack array; must use kmalloc */
+		ret = -ENOMEM;
 		bits = kmalloc(6 * size, GFP_KERNEL);
-	if (!bits)
-		goto out_nofds;
+		if (!bits)
+			goto out_nofds;
+	}
 	fds.in      = bits;
 	fds.out     = bits +   size;
 	fds.ex      = bits + 2*size;