[PATCH 2.4.33-rc1] repair __ide_dma_no_op breakage

[PATCH 2.4.33-rc1] repair __ide_dma_no_op breakage

[Mikael Pettersson]

On Fri, 16 Jun 2006 15:14:19 -0300, Marcelo Tosatti wrote:
>Summary of changes from v2.4.33-pre3 to v2.4.33-rc1
...
>Willy TARREAU:
...
>      ide: trying to enable DMA may cause an oops

This patch to ide-dma.c defines a function 'int __ide_dma_no_op(void)'
and stores its address in function pointer fields with type
'int (*)(ide_drive_t*)'. Thus callers will call __ide_dma_no_op() with
more parameters than it expects.

This is invalid C and it will break horribly in some valid calling
conventions (in particular, when parameters are passed on the stack
and the callee not the caller pops them).

Furtunately the fix is simple: just define __ide_dma_no_op() with
the correct prototype (taking an unused ide_drive_t* parameter),
and drop the now redundant casts from the assignments. Also make
__ide_dma_no_op() 'static' as it is local to ide-dma.c.

Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>

diff -rupN linux-2.4.33-rc1/drivers/ide/ide-dma.c linux-2.4.33-rc1.ide-dma-no-op-fix/drivers/ide/ide-dma.c
--- linux-2.4.33-rc1/drivers/ide/ide-dma.c	2006-06-17 17:58:36.000000000 +0200
+++ linux-2.4.33-rc1.ide-dma-no-op-fix/drivers/ide/ide-dma.c	2006-06-17 18:12:31.000000000 +0200
@@ -572,7 +572,7 @@ static int dma_timer_expiry (ide_drive_t
  *	This empty function prevents non-DMA controllers from causing an oops.
  */
 
-int __ide_dma_no_op (void)
+static int __ide_dma_no_op (ide_drive_t *ignored)
 {
 	return 0;
 }
@@ -1235,11 +1235,11 @@ EXPORT_SYMBOL_GPL(ide_setup_dma);
 void ide_setup_no_dma (ide_hwif_t *hwif)
 {
 	if (!hwif->ide_dma_off_quietly)
-		hwif->ide_dma_off_quietly = (int (*)(ide_drive_t *))&__ide_dma_no_op;
+		hwif->ide_dma_off_quietly = &__ide_dma_no_op;
 	if (!hwif->ide_dma_host_off)
-		hwif->ide_dma_host_off = (int (*)(ide_drive_t *))&__ide_dma_no_op;
+		hwif->ide_dma_host_off = &__ide_dma_no_op;
 	if (!hwif->ide_dma_host_on)
-		hwif->ide_dma_host_on = (int (*)(ide_drive_t *))&__ide_dma_no_op;
+		hwif->ide_dma_host_on = &__ide_dma_no_op;
 }
 
 EXPORT_SYMBOL_GPL(ide_setup_no_dma);

Re: [PATCH 2.4.33-rc1] repair __ide_dma_no_op breakage

[Willy Tarreau]

Hi Mikael,

On Sat, Jun 17, 2006 at 10:47:53PM +0200, Mikael Pettersson wrote:
> On Fri, 16 Jun 2006 15:14:19 -0300, Marcelo Tosatti wrote:
> >Summary of changes from v2.4.33-pre3 to v2.4.33-rc1
> ...
> >Willy TARREAU:
> ...
> >      ide: trying to enable DMA may cause an oops
> 
> This patch to ide-dma.c defines a function 'int __ide_dma_no_op(void)'
> and stores its address in function pointer fields with type
> 'int (*)(ide_drive_t*)'. Thus callers will call __ide_dma_no_op() with
> more parameters than it expects.
>
> This is invalid C and it will break horribly in some valid calling
> conventions (in particular, when parameters are passed on the stack
> and the callee not the caller pops them).

I 100% agree with your analysis and you fix, but just out of curiosity,
when could we encounter such circumstances ? On specific archs or when
functions are declared in a certain manner ? I've always learned that
in C, it's always the caller which pops, reason why var args are possible
and the args type checking is very loose.

> Furtunately the fix is simple: just define __ide_dma_no_op() with
> the correct prototype (taking an unused ide_drive_t* parameter),
> and drop the now redundant casts from the assignments. Also make
> __ide_dma_no_op() 'static' as it is local to ide-dma.c.

Thank you very much, I've queued it in -upstream. Marcelo, I've updated
the URL, please use http://git.1wt.eu/linux-2.4-upstream.git as of now.

Cheers,
Willy

> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
> 
> diff -rupN linux-2.4.33-rc1/drivers/ide/ide-dma.c linux-2.4.33-rc1.ide-dma-no-op-fix/drivers/ide/ide-dma.c
> --- linux-2.4.33-rc1/drivers/ide/ide-dma.c	2006-06-17 17:58:36.000000000 +0200
> +++ linux-2.4.33-rc1.ide-dma-no-op-fix/drivers/ide/ide-dma.c	2006-06-17 18:12:31.000000000 +0200
> @@ -572,7 +572,7 @@ static int dma_timer_expiry (ide_drive_t
>   *	This empty function prevents non-DMA controllers from causing an oops.
>   */
>  
> -int __ide_dma_no_op (void)
> +static int __ide_dma_no_op (ide_drive_t *ignored)
>  {
>  	return 0;
>  }
> @@ -1235,11 +1235,11 @@ EXPORT_SYMBOL_GPL(ide_setup_dma);
>  void ide_setup_no_dma (ide_hwif_t *hwif)
>  {
>  	if (!hwif->ide_dma_off_quietly)
> -		hwif->ide_dma_off_quietly = (int (*)(ide_drive_t *))&__ide_dma_no_op;
> +		hwif->ide_dma_off_quietly = &__ide_dma_no_op;
>  	if (!hwif->ide_dma_host_off)
> -		hwif->ide_dma_host_off = (int (*)(ide_drive_t *))&__ide_dma_no_op;
> +		hwif->ide_dma_host_off = &__ide_dma_no_op;
>  	if (!hwif->ide_dma_host_on)
> -		hwif->ide_dma_host_on = (int (*)(ide_drive_t *))&__ide_dma_no_op;
> +		hwif->ide_dma_host_on = &__ide_dma_no_op;
>  }
>  
>  EXPORT_SYMBOL_GPL(ide_setup_no_dma);

Re: [PATCH 2.4.33-rc1] repair __ide_dma_no_op breakage

[Mikael Pettersson]

On Sat, 17 Jun 2006 23:26:12 +0200, Willy Tarreau wrote:
>> This patch to ide-dma.c defines a function 'int __ide_dma_no_op(void)'
>> and stores its address in function pointer fields with type
>> 'int (*)(ide_drive_t*)'. Thus callers will call __ide_dma_no_op() with
>> more parameters than it expects.
>>
>> This is invalid C and it will break horribly in some valid calling
>> conventions (in particular, when parameters are passed on the stack
>> and the callee not the caller pops them).
>
>I 100% agree with your analysis and you fix, but just out of curiosity,
>when could we encounter such circumstances ? On specific archs or when
>functions are declared in a certain manner ?

I suspect that no currently supported architecure is affected as long
as default calling conventions are used, but i386 would be affected if
__attribute__((stdcall)) is in effect. I suspect m68k is in a similar
situation as i386.

> I've always learned that
>in C, it's always the caller which pops, reason why var args are possible
>and the args type checking is very loose.

What you describe is the typical implementation, but it is a historical
accident and not necessarily the best choice from a performance
perspective. Before ANSI-C introduced prototypes, there was no way to
distinguish variadic from non-variadic functions. Thus all functions
had to use the same conventions, which typically were caller-pops since
that's what's needed for variadic functions. With prototypes it's usually
better to use a callee-pops convention, since that tends to reduce code
space and it allows for tailcalls without blowing the stack. Unfortunately
binary compatibility with pre-ANSI-C object code meant that most if not
all ABIs kept their pre-ANSI-C rules, i.e. caller-pops.

/Mikael