Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Roman Zippel]

Hi,

On Thu, 4 Jan 2007, Al Viro wrote:

> PS: what would be the sane strategy for timer series merge, BTW?

PPS: I still don't like it. It fixes a rather theoretical problem with 
absolutely no practical relevance.
PPPS: type safety is also possible with container_of(), the prototype 
patch below demonstrates how to check that the signature matches and 
additionally it doesn't require to convert everything at once. More 
sophisticated checks can be done by putting this information into a 
separate section, from where it can be extracted at compile/run time.

bye, Roman

---
 include/linux/timer.h |   24 
 kernel/timer.c|   18 +-
 kernel/workqueue.c|   24 ++--
 3 files changed, 51 insertions(+), 15 deletions(-)

Index: linux-2.6-git/include/linux/timer.h
===
--- linux-2.6-git.orig/include/linux/timer.h2007-01-06 20:45:02.0 
+0100
+++ linux-2.6-git/include/linux/timer.h 2007-01-06 21:00:07.0 +0100
@@ -13,16 +13,40 @@ struct timer_list {
 
void (*function)(unsigned long);
unsigned long data;
+   unsigned long _sig;
 
struct tvec_t_base_s *base;
 };
 
+#define __timer_sig(type, member) ((offsetof(type, member) << 16) | 
sizeof(type))
+
+#define __TIMER_OLD_SIG(0xa5005a)
+
+typedef void timer_cb_t(struct timer_list *);
+
+extern void __timer_init(struct timer_list *t, timer_cb_t *func, long sig);
+
+#define timer_init(ptr, member, func)  \
+   __timer_init(&(ptr)->member, func,  \
+__timer_sig(typeof(*ptr), member))
+
+#define timer_of(ptr, type, member) ({ \
+   const struct timer_list *_t = (ptr);\
+   if (_t->_sig != __timer_sig(type, member)) {\
+   WARN_ON(_t->_sig != -__timer_sig(type, member));\
+   return; \
+   }   \
+   container_of(ptr, type, member);\
+})
+
+
 extern struct tvec_t_base_s boot_tvec_bases;
 
 #define TIMER_INITIALIZER(_function, _expires, _data) {\
.function = (_function),\
.expires = (_expires),  \
.data = (_data),\
+   ._sig = __TIMER_OLD_SIG,\
.base = _tvec_bases,   \
}
 
Index: linux-2.6-git/kernel/timer.c
===
--- linux-2.6-git.orig/kernel/timer.c   2007-01-06 20:45:02.0 +0100
+++ linux-2.6-git/kernel/timer.c2007-01-06 21:00:07.0 +0100
@@ -226,6 +226,11 @@ static void internal_add_timer(tvec_base
unsigned long idx = expires - base->timer_jiffies;
struct list_head *vec;
 
+   if (!timer->_sig) {
+   WARN_ON(1);
+   timer->_sig = __TIMER_OLD_SIG;
+   }
+
if (idx < TVR_SIZE) {
int i = expires & TVR_MASK;
vec = base->tv1.vec + i;
@@ -271,11 +276,22 @@ static void internal_add_timer(tvec_base
  */
 void fastcall init_timer(struct timer_list *timer)
 {
+   timer->_sig = __TIMER_OLD_SIG;
timer->entry.next = NULL;
timer->base = __raw_get_cpu_var(tvec_bases);
 }
 EXPORT_SYMBOL(init_timer);
 
+extern void __timer_init(struct timer_list *t, timer_cb_t *func, long sig)
+{
+   t->function = (void *)func;
+   t->_sig = -sig;
+   func(t);
+   t->_sig = sig;
+   t->entry.next = NULL;
+   t->base = __raw_get_cpu_var(tvec_bases);
+}
+
 static inline void detach_timer(struct timer_list *timer,
int clear_pending)
 {
@@ -567,7 +583,7 @@ static inline void __run_timers(tvec_bas
 
timer = list_entry(head->next,struct timer_list,entry);
fn = timer->function;
-   data = timer->data;
+   data = timer->_sig == __TIMER_OLD_SIG ? timer->data : 
(unsigned long)timer;
 
set_running_timer(base, timer);
detach_timer(timer, 1);
Index: linux-2.6-git/kernel/workqueue.c
===
--- linux-2.6-git.orig/kernel/workqueue.c   2007-01-06 19:51:40.0 
+0100
+++ linux-2.6-git/kernel/workqueue.c2007-01-06 21:02:59.0 +0100
@@ -218,9 +218,9 @@ int fastcall queue_work(struct workqueue
 }
 EXPORT_SYMBOL_GPL(queue_work);
 
-static void delayed_work_timer_fn(unsigned long __data)
+static void delayed_work_timer_fn(struct timer_list *timer)
 {
-   struct delayed_work *dwork = (struct delayed_work *)__data;
+   struct delayed_work *dwork = timer_of(timer, struct 

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Roman Zippel]

Hi,

On Thu, 4 Jan 2007, Al Viro wrote:

 PS: what would be the sane strategy for timer series merge, BTW?

PPS: I still don't like it. It fixes a rather theoretical problem with 
absolutely no practical relevance.
PPPS: type safety is also possible with container_of(), the prototype 
patch below demonstrates how to check that the signature matches and 
additionally it doesn't require to convert everything at once. More 
sophisticated checks can be done by putting this information into a 
separate section, from where it can be extracted at compile/run time.

bye, Roman

---
 include/linux/timer.h |   24 
 kernel/timer.c|   18 +-
 kernel/workqueue.c|   24 ++--
 3 files changed, 51 insertions(+), 15 deletions(-)

Index: linux-2.6-git/include/linux/timer.h
===
--- linux-2.6-git.orig/include/linux/timer.h2007-01-06 20:45:02.0 
+0100
+++ linux-2.6-git/include/linux/timer.h 2007-01-06 21:00:07.0 +0100
@@ -13,16 +13,40 @@ struct timer_list {
 
void (*function)(unsigned long);
unsigned long data;
+   unsigned long _sig;
 
struct tvec_t_base_s *base;
 };
 
+#define __timer_sig(type, member) ((offsetof(type, member)  16) | 
sizeof(type))
+
+#define __TIMER_OLD_SIG(0xa5005a)
+
+typedef void timer_cb_t(struct timer_list *);
+
+extern void __timer_init(struct timer_list *t, timer_cb_t *func, long sig);
+
+#define timer_init(ptr, member, func)  \
+   __timer_init((ptr)-member, func,  \
+__timer_sig(typeof(*ptr), member))
+
+#define timer_of(ptr, type, member) ({ \
+   const struct timer_list *_t = (ptr);\
+   if (_t-_sig != __timer_sig(type, member)) {\
+   WARN_ON(_t-_sig != -__timer_sig(type, member));\
+   return; \
+   }   \
+   container_of(ptr, type, member);\
+})
+
+
 extern struct tvec_t_base_s boot_tvec_bases;
 
 #define TIMER_INITIALIZER(_function, _expires, _data) {\
.function = (_function),\
.expires = (_expires),  \
.data = (_data),\
+   ._sig = __TIMER_OLD_SIG,\
.base = boot_tvec_bases,   \
}
 
Index: linux-2.6-git/kernel/timer.c
===
--- linux-2.6-git.orig/kernel/timer.c   2007-01-06 20:45:02.0 +0100
+++ linux-2.6-git/kernel/timer.c2007-01-06 21:00:07.0 +0100
@@ -226,6 +226,11 @@ static void internal_add_timer(tvec_base
unsigned long idx = expires - base-timer_jiffies;
struct list_head *vec;
 
+   if (!timer-_sig) {
+   WARN_ON(1);
+   timer-_sig = __TIMER_OLD_SIG;
+   }
+
if (idx  TVR_SIZE) {
int i = expires  TVR_MASK;
vec = base-tv1.vec + i;
@@ -271,11 +276,22 @@ static void internal_add_timer(tvec_base
  */
 void fastcall init_timer(struct timer_list *timer)
 {
+   timer-_sig = __TIMER_OLD_SIG;
timer-entry.next = NULL;
timer-base = __raw_get_cpu_var(tvec_bases);
 }
 EXPORT_SYMBOL(init_timer);
 
+extern void __timer_init(struct timer_list *t, timer_cb_t *func, long sig)
+{
+   t-function = (void *)func;
+   t-_sig = -sig;
+   func(t);
+   t-_sig = sig;
+   t-entry.next = NULL;
+   t-base = __raw_get_cpu_var(tvec_bases);
+}
+
 static inline void detach_timer(struct timer_list *timer,
int clear_pending)
 {
@@ -567,7 +583,7 @@ static inline void __run_timers(tvec_bas
 
timer = list_entry(head-next,struct timer_list,entry);
fn = timer-function;
-   data = timer-data;
+   data = timer-_sig == __TIMER_OLD_SIG ? timer-data : 
(unsigned long)timer;
 
set_running_timer(base, timer);
detach_timer(timer, 1);
Index: linux-2.6-git/kernel/workqueue.c
===
--- linux-2.6-git.orig/kernel/workqueue.c   2007-01-06 19:51:40.0 
+0100
+++ linux-2.6-git/kernel/workqueue.c2007-01-06 21:02:59.0 +0100
@@ -218,9 +218,9 @@ int fastcall queue_work(struct workqueue
 }
 EXPORT_SYMBOL_GPL(queue_work);
 
-static void delayed_work_timer_fn(unsigned long __data)
+static void delayed_work_timer_fn(struct timer_list *timer)
 {
-   struct delayed_work *dwork = (struct delayed_work *)__data;
+   struct delayed_work *dwork = timer_of(timer, struct delayed_work, 
timer);

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Bodo Eggert]

Eric Sandeen <[EMAIL PROTECTED]> wrote:
> Andrew Morton wrote:

>> +++ a/fs/bad_inode.c

>> -static int return_EIO(void)
>> +static long return_EIO(void)

> What about ops that return loff_t (64 bits) on 32-bit arches and stuff
> it into 2 registers

*If* it uses an additional register for the high bits, it will set e.g.:
EDX << 32 | EAX == (s64) -EIO
 and therefore
EAX == -EIO // < -MAXLONGINT-1
EDX == -1

EAX will be the return register for s32. Therefore you can use one function
for both cases on i386:

long long f()
{
return -42;
}

long  (*l )() = (void*)f; // hide warning
long long (*ll)() =f;

int main(){
printf("%ld %lld\n", l(), ll());
}

> I'm still not convinced that this is the best place to be clever :)

ACK, not too clever, but not too stupid, too. Having #ifdef I386 etc.
isn't nice, and something like this shouldn't be arch-specific.
OTOH, C calling convention allows having a different argument signature,
so you can safely use it. It's a feature.
-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.

http://david.woodhou.se/why-not-spf.html
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Arjan van de Ven]

On Thu, 2007-01-04 at 23:52 +, Al Viro wrote:
> On Thu, Jan 04, 2007 at 03:21:06PM -0800, Mitchell Blank Jr wrote:
> > Linus Torvalds wrote:
> > > Well, that probably would work, but it's also true that returning a 
> > > 64-bit 
> > > value on a 32-bit platform really _does_ depend on more than the size.
> > 
> > Yeah, obviously this is restricted to the signed-integer case.  My point
> > was just that you could have the compiler figure out which variant to pick
> > for loff_t automatically.
> > 
> > > "let's not play tricks with function types at all".
> > 
> > I think I agree.  The real (but harder) fix for the wasted space issue
> > would be to get the toolchain to automatically combine functions that
> > end up compiling into identical assembly.
> 
> Can't do.
> 

you could if it's static and never has it's address taken (but that's
not the case here)


> You _can_ compile g into jump to f, but that's it.  And that, AFAICS,
> is what gcc does.

I thought we actually disabled that in the kernel (it makes oopses
harder to read)



-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via 
http://www.linuxfirmwarekit.org

-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Arjan van de Ven]

On Thu, 2007-01-04 at 23:52 +, Al Viro wrote:
 On Thu, Jan 04, 2007 at 03:21:06PM -0800, Mitchell Blank Jr wrote:
  Linus Torvalds wrote:
   Well, that probably would work, but it's also true that returning a 
   64-bit 
   value on a 32-bit platform really _does_ depend on more than the size.
  
  Yeah, obviously this is restricted to the signed-integer case.  My point
  was just that you could have the compiler figure out which variant to pick
  for loff_t automatically.
  
   let's not play tricks with function types at all.
  
  I think I agree.  The real (but harder) fix for the wasted space issue
  would be to get the toolchain to automatically combine functions that
  end up compiling into identical assembly.
 
 Can't do.
 

you could if it's static and never has it's address taken (but that's
not the case here)


 You _can_ compile g into jump to f, but that's it.  And that, AFAICS,
 is what gcc does.

I thought we actually disabled that in the kernel (it makes oopses
harder to read)



-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via 
http://www.linuxfirmwarekit.org

-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Bodo Eggert]

Eric Sandeen [EMAIL PROTECTED] wrote:
 Andrew Morton wrote:

 +++ a/fs/bad_inode.c

 -static int return_EIO(void)
 +static long return_EIO(void)

 What about ops that return loff_t (64 bits) on 32-bit arches and stuff
 it into 2 registers

*If* it uses an additional register for the high bits, it will set e.g.:
EDX  32 | EAX == (s64) -EIO
 and therefore
EAX == -EIO //  -MAXLONGINT-1
EDX == -1

EAX will be the return register for s32. Therefore you can use one function
for both cases on i386:

long long f()
{
return -42;
}

long  (*l )() = (void*)f; // hide warning
long long (*ll)() =f;

int main(){
printf(%ld %lld\n, l(), ll());
}

 I'm still not convinced that this is the best place to be clever :)

ACK, not too clever, but not too stupid, too. Having #ifdef I386 etc.
isn't nice, and something like this shouldn't be arch-specific.
OTOH, C calling convention allows having a different argument signature,
so you can safely use it. It's a feature.
-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.

http://david.woodhou.se/why-not-spf.html
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Al Viro]

On Thu, Jan 04, 2007 at 03:21:06PM -0800, Mitchell Blank Jr wrote:
> Linus Torvalds wrote:
> > Well, that probably would work, but it's also true that returning a 64-bit 
> > value on a 32-bit platform really _does_ depend on more than the size.
> 
> Yeah, obviously this is restricted to the signed-integer case.  My point
> was just that you could have the compiler figure out which variant to pick
> for loff_t automatically.
> 
> > "let's not play tricks with function types at all".
> 
> I think I agree.  The real (but harder) fix for the wasted space issue
> would be to get the toolchain to automatically combine functions that
> end up compiling into identical assembly.

Can't do.

int f(void)
{
return 0;
}

int g(void)
{
return 0;
}

int is_f(int (*p)(void))
{
return p == f;
}

main()
{
printf("%d %d\n", is_f(f), is_f(g));
}

would better produce
1 0
for anything resembling a sane C compiler.  Comparing pointers to
functions for equality is a well-defined operation and it's not
to be messed with.

You _can_ compile g into jump to f, but that's it.  And that, AFAICS,
is what gcc does.
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Linus Torvalds wrote:

> On Thu, 4 Jan 2007, Andrew Morton wrote:
>   
>> That's what I currently have queued.  It increases bad_inode.o text from 
>> 200-odd bytes to 700-odd :(
>> 
> Then I think we're ok. We do care about bytes, but we care more about 
> bytes that actually ever hit the icache or dcache, and this will 
> effectively do neither.
>
>   
Oh good.  Resolution?  ;-)

Andrew, if you stick with what you've got, you might want this on top of it.

Mostly cosmetic, making placement of * for pointers consistently " *foo"
not "* foo," (was a mishmash before, from cut-n-paste), but also making 
.poll return POLLERR.

Thanks,
-Eric

Signed-off-by: Eric Sandeen <[EMAIL PROTECTED]>

Index: linux-2.6.19/fs/bad_inode.c
===
--- linux-2.6.19.orig/fs/bad_inode.c
+++ linux-2.6.19/fs/bad_inode.c
@@ -46,18 +46,17 @@ static ssize_t bad_file_aio_write(struct
return -EIO;
 }
 
-static int bad_file_readdir(struct file * filp, void * dirent,
-   filldir_t filldir)
+static int bad_file_readdir(struct file *filp, void *dirent, filldir_t filldir)
 {
return -EIO;
 }
 
 static unsigned int bad_file_poll(struct file *filp, poll_table *wait)
 {
-   return -EIO;
+   return POLLERR;
 }
 
-static int bad_file_ioctl (struct inode * inode, struct file * filp,
+static int bad_file_ioctl (struct inode *inode, struct file *filp,
unsigned int cmd, unsigned long arg)
 {
return -EIO;
@@ -75,12 +74,12 @@ static long bad_file_compat_ioctl(struct
return -EIO;
 }
 
-static int bad_file_mmap(struct file * file, struct vm_area_struct * vma)
+static int bad_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
return -EIO;
 }
 
-static int bad_file_open(struct inode * inode, struct file * filp)
+static int bad_file_open(struct inode *inode, struct file *filp)
 {
return -EIO;
 }
@@ -90,12 +89,12 @@ static int bad_file_flush(struct file *f
return -EIO;
 }
 
-static int bad_file_release(struct inode * inode, struct file * filp)
+static int bad_file_release(struct inode *inode, struct file *filp)
 {
return -EIO;
 }
 
-static int bad_file_fsync(struct file * file, struct dentry *dentry,
+static int bad_file_fsync(struct file *file, struct dentry *dentry,
int datasync)
 {
return -EIO;
@@ -140,7 +139,7 @@ static int bad_file_check_flags(int flag
return -EIO;
 }
 
-static int bad_file_dir_notify(struct file * file, unsigned long arg)
+static int bad_file_dir_notify(struct file *file, unsigned long arg)
 {
return -EIO;
 }
@@ -194,54 +193,54 @@ static const struct file_operations bad_
.splice_read= bad_file_splice_read,
 };
 
-static int bad_inode_create (struct inode * dir, struct dentry * dentry,
+static int bad_inode_create (struct inode *dir, struct dentry *dentry,
int mode, struct nameidata *nd)
 {
return -EIO;
 }
 
-static struct dentry *bad_inode_lookup(struct inode * dir,
+static struct dentry *bad_inode_lookup(struct inode *dir,
struct dentry *dentry, struct nameidata *nd)
 {
return ERR_PTR(-EIO);
 }
 
-static int bad_inode_link (struct dentry * old_dentry, struct inode * dir,
+static int bad_inode_link (struct dentry *old_dentry, struct inode *dir,
struct dentry *dentry)
 {
return -EIO;
 }
 
-static int bad_inode_unlink(struct inode * dir, struct dentry *dentry)
+static int bad_inode_unlink(struct inode *dir, struct dentry *dentry)
 {
return -EIO;
 }
 
-static int bad_inode_symlink (struct inode * dir, struct dentry *dentry,
-   const char * symname)
+static int bad_inode_symlink (struct inode *dir, struct dentry *dentry,
+   const char *symname)
 {
return -EIO;
 }
 
-static int bad_inode_mkdir(struct inode * dir, struct dentry * dentry,
+static int bad_inode_mkdir(struct inode *dir, struct dentry *dentry,
int mode)
 {
return -EIO;
 }
 
-static int bad_inode_rmdir (struct inode * dir, struct dentry *dentry)
+static int bad_inode_rmdir (struct inode *dir, struct dentry *dentry)
 {
return -EIO;
 }
 
-static int bad_inode_mknod (struct inode * dir, struct dentry *dentry,
+static int bad_inode_mknod (struct inode *dir, struct dentry *dentry,
int mode, dev_t rdev)
 {
return -EIO;
 }
 
-static int bad_inode_rename (struct inode * old_dir, struct dentry *old_dentry,
-   struct inode * new_dir, struct dentry *new_dentry)
+static int bad_inode_rename (struct inode *old_dir, struct dentry *old_dentry,
+   struct inode *new_dir, struct dentry *new_dentry)
 {
return -EIO;
 }
@@ -337,7 +336,7 @@ static struct inode_operations bad_inode
  * on it to fail from this point on.
  */
  
-void make_bad_inode(struct inode * inode) 
+void make_bad_inode(struct inode *inode) 
 {

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Linus Torvalds]

On Thu, 4 Jan 2007, Andrew Morton wrote:
> 
> That's what I currently have queued.  It increases bad_inode.o text from 
> 200-odd bytes to 700-odd :(

Then I think we're ok. We do care about bytes, but we care more about 
bytes that actually ever hit the icache or dcache, and this will 
effectively do neither.

> 

That one should be ok. System calls by definition have the same return 
type, since they all have the same call site. So that one really does fit 
the "argument types differ, but the return type is the same" case. 

Linus
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Andrew Morton]

On Thu, 4 Jan 2007 14:35:09 -0800 (PST)
Linus Torvalds <[EMAIL PROTECTED]> wrote:

> Anybody want to send in the patch that just generates separate versions 
> for
> 
>   loff_t eio_llseek(struct file *file, loff_t offset, int origin)
>   {
>   return -EIO;
>   }
> 
>   int eio_readdir(struct file *filp, void *dirent, filldir_t filldir)
>   {
>   return -EIO;
>   ..
> 

That's what I currently have queued.  It increases bad_inode.o text from 
200-odd bytes to 700-odd :(


-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Mitchell Blank Jr]

Linus Torvalds wrote:
> Well, that probably would work, but it's also true that returning a 64-bit 
> value on a 32-bit platform really _does_ depend on more than the size.

Yeah, obviously this is restricted to the signed-integer case.  My point
was just that you could have the compiler figure out which variant to pick
for loff_t automatically.

> "let's not play tricks with function types at all".

I think I agree.  The real (but harder) fix for the wasted space issue
would be to get the toolchain to automatically combine functions that
end up compiling into identical assembly.

-Mitch
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Linus Torvalds wrote:
> Anybody want to send in the patch that just generates separate versions 
> for
> 
>   loff_t eio_llseek(struct file *file, loff_t offset, int origin)
>   {
>   return -EIO;
>   }
> 
>   int eio_readdir(struct file *filp, void *dirent, filldir_t filldir)
>   {
>   return -EIO;
>   ..
> 
> and so on?

That's more or less what I sent at http://lkml.org/lkml/2007/1/3/244

+static int bad_file_readdir(struct file * filp, void * dirent,
+   filldir_t filldir)
+{
+   return -EIO;
+}

... etc

Thanks,
-Eric
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Linus Torvalds]

On Thu, 4 Jan 2007, Mitchell Blank Jr wrote:
> 
> I don't think you need to do fancy #ifdef's:
> 
> static s32 return_eio_32(void) { return -EIO; }
> static s64 return_eio_64(void) { return -EIO; }
> extern void return_eio_unknown(void);   /* Doesn't exist */
> #define return_eio(type)((sizeof(type) == 4)  \
>   ? ((void *) return_eio_32)  \
>   : ((sizeof(type) == 8)  \
>   ? ((void *) return_eio_64)  \
>   : ((void *) return_eio_unknown)))

Well, that probably would work, but it's also true that returning a 64-bit 
value on a 32-bit platform really _does_ depend on more than the size.

For an example of this, try compiling this:

long long a(void)
{
return -1;
}

struct dummy { int a, b };

struct dummy b(void)
{
struct dummy retval = { -1 , -1 };
return retval;
}

on x86.

Now, I don't think we actually have anything like this in the kernel, and 
your example is likely to work very well in practice, but once we start 
doing tricks like this, I actually think it's getting easier to just say 
"let's not play tricks with function types at all".

Anybody want to send in the patch that just generates separate versions 
for

loff_t eio_llseek(struct file *file, loff_t offset, int origin)
{
return -EIO;
}

int eio_readdir(struct file *filp, void *dirent, filldir_t filldir)
{
return -EIO;
..

and so on?

Linus
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Mitchell Blank Jr]

Al Viro wrote:
> At least 3 versions, unless you want to mess with ifdefs to reduce them to
> two.

I don't think you need to do fancy #ifdef's:

static s32 return_eio_32(void) { return -EIO; }
static s64 return_eio_64(void) { return -EIO; }
extern void return_eio_unknown(void);   /* Doesn't exist */
#define return_eio(type)((sizeof(type) == 4)\
? ((void *) return_eio_32)  \
: ((sizeof(type) == 8)  \
? ((void *) return_eio_64)  \
: ((void *) return_eio_unknown)))

-Mitch
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Al Viro]

On Thu, Jan 04, 2007 at 01:30:47PM -0800, Linus Torvalds wrote:
 
> I'll happily cast away arguments that aren't used, but I'm not sure that 
> we ever should cast different return values (not "int" vs "long", but also 
> not "loff_t" etc). 
> 
> On 32-bit architectures, 64-bit entities may be returned totally different 
> ways (ie things like "caller allocates space for them and passes in a 
> magic pointer to the return value as the first _real_ argument").
> 
> So with my previous email, I was definitely _not_ trying to say that 
> casting function pointers is ok. In practice it is ok when the _arguments_ 
> differ, but not necessarily when the _return-type_ differs.
> 
> I was cc'd into the discussion late, so I didn't realize that we 
> apparently already have a situation where changing the return value to 
> "long" might make a difference. If so, I agree that we shouldn't do this 
> at all (although Andrew's change to "long" seems perfectly fine as a "make 
> old cases continue to work" patch if it actually matters).

We do.
loff_t (*llseek) (struct file *, loff_t, int);
...
int (*readdir) (struct file *, void *, filldir_t);

static const struct file_operations bad_file_ops =
{
.llseek = EIO_ERROR,
...
.readdir= EIO_ERROR,


Moreover, we have int, loff_t, ssize_t and long, plus the unsigned variants.
At least 3 versions, unless you want to mess with ifdefs to reduce them to
two.
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Linus Torvalds wrote:
> 
> On Thu, 4 Jan 2007, Eric Sandeen wrote:
>> Andrew Morton wrote:
>>
>>> btw, couldn't we fix this bug with a simple old
>>>
>>> --- a/fs/bad_inode.c~a
>>> +++ a/fs/bad_inode.c
>>> @@ -15,7 +15,7 @@
>>>  #include 
>>>  #include 
>>>  
>>> -static int return_EIO(void)
>>> +static long return_EIO(void)
>>>  {
>>> return -EIO;
>>>  }
>>> _
>>>
>>> ?
>> What about ops that return loff_t (64 bits) on 32-bit arches and stuff
>> it into 2 registers
> 
> Do we actually have cases where we cast to a different return value?

Today, via the void * function casts in the bad file/inode ops, in
effect yes.

static int return_EIO(void)
{
return -EIO;
}

#define EIO_ERROR ((void *) (return_EIO))

...
.listxattr  = EIO_ERROR,

but listxattr is supposed to return a ssize_t, which is 64 bits on some
platforms, and only 32 bits get filled in thanks to the (void *) cast.
So we wind up with something other than the return value we expect...

Andrew's long suggestion breaks things the other way, with 64-bit
returning ops on 32-bit arches which again only pick up the first 32
bits thanks to the (void *) cast.

If we're really happy with casting away the function arguments (which
are not -used- in the bad_foo ops anyway), then I'd maybe suggest going
back to my first try at this thing:

static int return_EIO_int(void)
{
return -EIO;
}
#define EIO_ERROR_INT ((void *) (return_EIO_int))

static struct inode_operations bad_inode_ops =
{
.create = EIO_ERROR_INT,
...etc...

which is most like what we have today, except with specific return types.

-Eric
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Linus Torvalds]

On Thu, 4 Jan 2007, Eric Sandeen wrote:
> Andrew Morton wrote:
> 
> > btw, couldn't we fix this bug with a simple old
> > 
> > --- a/fs/bad_inode.c~a
> > +++ a/fs/bad_inode.c
> > @@ -15,7 +15,7 @@
> >  #include 
> >  #include 
> >  
> > -static int return_EIO(void)
> > +static long return_EIO(void)
> >  {
> > return -EIO;
> >  }
> > _
> > 
> > ?
> 
> What about ops that return loff_t (64 bits) on 32-bit arches and stuff
> it into 2 registers

Do we actually have cases where we cast to a different return value?

I'll happily cast away arguments that aren't used, but I'm not sure that 
we ever should cast different return values (not "int" vs "long", but also 
not "loff_t" etc). 

On 32-bit architectures, 64-bit entities may be returned totally different 
ways (ie things like "caller allocates space for them and passes in a 
magic pointer to the return value as the first _real_ argument").

So with my previous email, I was definitely _not_ trying to say that 
casting function pointers is ok. In practice it is ok when the _arguments_ 
differ, but not necessarily when the _return-type_ differs.

I was cc'd into the discussion late, so I didn't realize that we 
apparently already have a situation where changing the return value to 
"long" might make a difference. If so, I agree that we shouldn't do this 
at all (although Andrew's change to "long" seems perfectly fine as a "make 
old cases continue to work" patch if it actually matters).

Linus
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Andrew Morton wrote:
> On Thu, 04 Jan 2007 15:04:17 -0600
> Eric Sandeen <[EMAIL PROTECTED]> wrote:
> 
>> Andrew Morton wrote:
>>> On Thu, 4 Jan 2007 20:24:12 +
>>> Al Viro <[EMAIL PROTECTED]> wrote:
>>>
 So my main issue with fs/bad_inode.c is not even cast per se; it's that
 cast is to void *.
>>> But Eric's latest patch is OK in that regard, isn't it?  It might confuse
>>> parsers (in fixable ways), but it is type-correct and has no casts.  (Well,
>>> it kinda has an link-time cast).
>> Even if it is, I'm starting to wonder if all this tricksiness is really
>> worth it for 400 bytes or so.  :)
>>
> 
> Ah, but it's a learning opportunity!

*grin*

> btw, couldn't we fix this bug with a simple old
> 
> --- a/fs/bad_inode.c~a
> +++ a/fs/bad_inode.c
> @@ -15,7 +15,7 @@
>  #include 
>  #include 
>  
> -static int return_EIO(void)
> +static long return_EIO(void)
>  {
>   return -EIO;
>  }
> _
> 
> ?

What about ops that return loff_t (64 bits) on 32-bit arches and stuff
it into 2 registers

#include 
#include 
#include 

static long return_EIO(void)
{
return -EIO;
}

#define EIO_ERROR ((void *) (return_EIO))

int main(int argc, char ** argv)
{
loff_t error;
loff_t (*fn_ptr) (int);

fn_ptr = EIO_ERROR;

error = fn_ptr(0);
printf("and... error is %lld\n", error);
return 0;
}

[root]# ./ssize_eio
and... error is 8589934587
[root]# uname -m
i686

I'm still not convinced that this is the best place to be clever :)

-Eric
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Andrew Morton]

On Thu, 04 Jan 2007 15:04:17 -0600
Eric Sandeen <[EMAIL PROTECTED]> wrote:

> Andrew Morton wrote:
> > On Thu, 4 Jan 2007 20:24:12 +
> > Al Viro <[EMAIL PROTECTED]> wrote:
> > 
> >> So my main issue with fs/bad_inode.c is not even cast per se; it's that
> >> cast is to void *.
> > 
> > But Eric's latest patch is OK in that regard, isn't it?  It might confuse
> > parsers (in fixable ways), but it is type-correct and has no casts.  (Well,
> > it kinda has an link-time cast).
> 
> Even if it is, I'm starting to wonder if all this tricksiness is really
> worth it for 400 bytes or so.  :)
> 

Ah, but it's a learning opportunity!

btw, couldn't we fix this bug with a simple old

--- a/fs/bad_inode.c~a
+++ a/fs/bad_inode.c
@@ -15,7 +15,7 @@
 #include 
 #include 
 
-static int return_EIO(void)
+static long return_EIO(void)
 {
return -EIO;
 }
_

?
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Andrew Morton wrote:
> On Thu, 4 Jan 2007 20:24:12 +
> Al Viro <[EMAIL PROTECTED]> wrote:
> 
>> So my main issue with fs/bad_inode.c is not even cast per se; it's that
>> cast is to void *.
> 
> But Eric's latest patch is OK in that regard, isn't it?  It might confuse
> parsers (in fixable ways), but it is type-correct and has no casts.  (Well,
> it kinda has an link-time cast).

Even if it is, I'm starting to wonder if all this tricksiness is really
worth it for 400 bytes or so.  :)

-Eric
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Andrew Morton]

On Thu, 4 Jan 2007 20:24:12 +
Al Viro <[EMAIL PROTECTED]> wrote:

> So my main issue with fs/bad_inode.c is not even cast per se; it's that
> cast is to void *.

But Eric's latest patch is OK in that regard, isn't it?  It might confuse
parsers (in fixable ways), but it is type-correct and has no casts.  (Well,
it kinda has an link-time cast).
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Al Viro]

On Thu, Jan 04, 2007 at 11:30:22AM -0800, Linus Torvalds wrote:
> 
> 
> On Thu, 4 Jan 2007, Al Viro wrote:
> > 
> > How about "makes call graph analysis easier"? ;-)  In principle, I have
> > no problem with force-casting, but it'd better be cast to the right
> > type...
> 
> Do we really care in the kernel? We simply never use function pointer 
> casts like this for anything non-trivial, so if the graph analysis just 
> doesn't work for those cases, do we really even care?

Umm...  Let me put it that way - amount of things that can be done to
void * is much more than what can be done to function pointers.  So
keeping track of them gets easier if we never do casts to/from void *.
What's more, very few places in the kernel try to do that _and_ most
of those that do are simply too lazy to declare local variable with
the right type.  bad_inode.c covers most of what remains.

IMO we ought to start checking for that kind of stuff; note that we _still_
have strugglers from pt_regs removal where interrupt handler still takes
3 arguments, but we don't notice since argument of request_irq() is cast
to void * ;-/

That's local stuff; however, when trying to do non-local work (e.g. deduce
that foo() may be called from BH, bar() is always called from process
context, etc. _without_ fuckloads of annotations all over the place), the
ban on mixing void * with function pointers helps a _lot_.

So my main issue with fs/bad_inode.c is not even cast per se; it's that
cast is to void *.
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Linus Torvalds]

On Thu, 4 Jan 2007, Al Viro wrote:
> 
> PS: what would be the sane strategy for timer series merge, BTW?

I think Andrew may care more. I just care about it happening after 2.6.20.

Whether we can do it really early after that, or whether we should do it 
as the last thing just before releasing -rc1 (to avoid having other people 
have to fix up conflicts during the merge window), who knows..

Linus
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Linus Torvalds]

On Thu, 4 Jan 2007, Al Viro wrote:
> 
> How about "makes call graph analysis easier"? ;-)  In principle, I have
> no problem with force-casting, but it'd better be cast to the right
> type...

Do we really care in the kernel? We simply never use function pointer 
casts like this for anything non-trivial, so if the graph analysis just 
doesn't work for those cases, do we really even care?

The only case I can _remember_ us doing this for is literally the 
error-returning functions, where the call graph finding them really 
doesn't matter, I think.

So I don't _object_ to that reason, I just wonder whether it's really a 
big issue..

Linus
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Mikael Pettersson]

On Thu, 04 Jan 2007 11:51:10 -0600, Eric Sandeen wrote:
>Also - is it ok to alias a function with one signature to a function with
>another signature?

NO!

Specific cases of type mismatches may work on many machines
(say different pointer types as long as they aren't accessed),
but in general this won't work since types affect calling conventions.
Abusing aliasing like this is exactly like casting a function
pointer to a different type that it actually has, and then invoking
it at that type: all bets are off.

Don't do this. Ever.

/Mikael
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Al Viro]

On Thu, Jan 04, 2007 at 07:14:51PM +, Al Viro wrote:
> On Thu, Jan 04, 2007 at 11:09:31AM -0800, Linus Torvalds wrote:
>  
> > But I'd argue we should only do it if there is an actual 
> > honest-to-goodness reason to do so.
> 
> How about "makes call graph analysis easier"? ;-)  In principle, I have
> no problem with force-casting, but it'd better be cast to the right
> type...
> 
> (And yes, there's a bunch of sparse-based fun in making dealing with
> call graph analysis and sane annotations needed for that).

PS: what would be the sane strategy for timer series merge, BTW?  It
touches a whole lot of files in rather trivial ways (see below for current
stat), but it's gradually mergable and after the first 4 chunks the rest
can go in independently (per-driver, if we want to go for insane length,
but most of those will be absolutely trivial and I'd rather lump them
into bigger groups).  And those 4 chunks in the beginning of series are
safe to merge at any point - result in guaranteed to be identical code...

 arch/alpha/kernel/srmcons.c|8 +--
 arch/arm/common/sharpsl_pm.c   |   10 ++--
 arch/arm/mach-iop32x/n2100.c   |5 +-
 arch/arm/mach-pxa/lubbock.c|   12 ++---
 arch/i386/kernel/time.c|6 +-
 arch/i386/kernel/tsc.c |5 +-
 arch/i386/mach-voyager/voyager_thread.c|5 +-
 arch/ia64/kernel/mca.c |   12 ++---
 arch/ia64/kernel/salinfo.c |5 +-
 arch/ia64/sn/kernel/bte.c  |7 +--
 arch/ia64/sn/kernel/bte_error.c|   17 ++
 arch/ia64/sn/kernel/huberror.c |2 -
 arch/ia64/sn/kernel/mca.c  |5 +-
 arch/ia64/sn/kernel/xpc_channel.c  |4 --
 arch/ia64/sn/kernel/xpc_main.c |   18 ++-
 arch/mips/lasat/picvue_proc.c  |5 +-
 arch/mips/sgi-ip22/ip22-reset.c|   20 +++-
 arch/mips/sgi-ip32/ip32-reset.c|   10 ++--
 arch/powerpc/oprofile/op_model_cell.c  |6 +-
 arch/powerpc/platforms/chrp/chrp.h |2 -
 arch/powerpc/platforms/chrp/setup.c|4 +-
 arch/powerpc/platforms/powermac/low_i2c.c  |7 +--
 arch/ppc/syslib/m8xx_wdt.c |   10 ++--
 arch/s390/mm/cmm.c |   23 +++--
 arch/sh/drivers/push-switch.c  |9 +--
 arch/um/drivers/net_kern.c |7 +--
 arch/x86_64/kernel/pci-calgary.c   |7 +--
 arch/xtensa/platform-iss/console.c |   10 +---
 arch/xtensa/platform-iss/network.c |   13 ++---
 block/as-iosched.c |7 +--
 block/cfq-iosched.c|   15 ++
 block/ll_rw_blk.c  |9 +--
 drivers/acpi/sbs.c |7 +--
 drivers/acpi/thermal.c |   26 +++---
 drivers/atm/ambassador.c   |   10 +---
 drivers/atm/firestream.c   |   10 +---
 drivers/atm/horizon.c  |   11 +---
 drivers/atm/idt77252.c |   14 ++---
 drivers/atm/lanai.c|7 +--
 drivers/atm/nicstar.c  |8 +--
 drivers/atm/suni.c |8 +--
 drivers/base/firmware_class.c  |   11 
 drivers/block/DAC960.c |8 +--
 drivers/block/DAC960.h |2 -
 drivers/block/cpqarray.c   |   10 +---
 drivers/block/swim3.c  |   43 +---
 drivers/block/ub.c |   22 ++--
 drivers/block/umem.c   |5 +-
 drivers/block/xd.c |4 +-
 drivers/block/xd.h |2 -
 drivers/bluetooth/bluecard_cs.c|7 +--
 drivers/bluetooth/hci_bcsp.c   |7 +--
 drivers/cdrom/aztcd.c  |   10 ++--
 drivers/cdrom/cdu31a.c |5 +-
 drivers/cdrom/cm206.c  |   11 ++--
 drivers/char/drm/via_dmablit.c |7 +--
 drivers/char/dtlk.c|7 +--
 drivers/char/epca.c|7 +--
 drivers/char/genrtc.c  |7 +--
 drivers/char/ip2/i2ellis.c |8 +--
 drivers/char/ip2/i2lib.c   |9 +--
 drivers/char/ipmi/ipmi_msghandler.c|4 +-
 drivers/char/ipmi/ipmi_si_intf.c   |5 +-
 drivers/char/moxa.c|   21 +++-
 drivers/char/mspec.c   |2 -
 drivers/char/n_r3964.c |9 +--
 drivers/char/nwbutton.c   

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Al Viro]

On Thu, Jan 04, 2007 at 11:09:31AM -0800, Linus Torvalds wrote:
 
> But I'd argue we should only do it if there is an actual 
> honest-to-goodness reason to do so.

How about "makes call graph analysis easier"? ;-)  In principle, I have
no problem with force-casting, but it'd better be cast to the right
type...

(And yes, there's a bunch of sparse-based fun in making dealing with
call graph analysis and sane annotations needed for that).
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Linus Torvalds]

On Thu, 4 Jan 2007, Andrew Morton wrote:

> On Thu, 04 Jan 2007 12:33:59 -0600
> Eric Sandeen <[EMAIL PROTECTED]> wrote:
> 
> > Andrew Morton wrote:
> > > On Thu, 04 Jan 2007 11:51:10 -0600
> > > Eric Sandeen <[EMAIL PROTECTED]> wrote:
> > 
> > >> Also - is it ok to alias a function with one signature to a function with
> > >> another signature?
> > > 
> > > Ordinarily I'd say no wucking fay, but that's effectively what we've been
> > > doing in there for ages, and it seems to work.
> > 
> > Hmm that gives me a lot of confidence ;-)  I'd hate to carry along bad
> > assumptions while we try to make this all kosher... but I'm willing to
> > defer to popular opinion on this one
> 
> yeah, I'm a bit wobbly about it.  Linus, what do you think?

I don't much care. If we ever find an architecture where it matters, we'll 
unalias them. In the meantime, we've for the longest time just known that 
calling conventions don't care about argument types on all the 
architectures we've been on, so we've aliased things to the same function.

But it's not very common, so we can stop doing it. 

But I'd argue we should only do it if there is an actual 
honest-to-goodness reason to do so. Usually it only matters for

 - return types are fundamentally different (FP vs integer vs pointer)

 - calling convention has callee popping the arguments (normal in Pascal, 
   very unusual in C, because it also breaks lots of historical code, 
   and is simply not workable with K C, where perfectly normal things 
   like "open()" take either two or three arguments without being 
   varargs).

In general, this just isn't an issue for the kernel. Other systems have 
had basically NO typing what-so-ever for functions, and use aliasing much 
more extensively. We only do it for a few ratehr rare things.

Linus
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Andrew Morton]

On Thu, 04 Jan 2007 12:33:59 -0600
Eric Sandeen <[EMAIL PROTECTED]> wrote:

> Andrew Morton wrote:
> > On Thu, 04 Jan 2007 11:51:10 -0600
> > Eric Sandeen <[EMAIL PROTECTED]> wrote:
> 
> >> Also - is it ok to alias a function with one signature to a function with
> >> another signature?
> > 
> > Ordinarily I'd say no wucking fay, but that's effectively what we've been
> > doing in there for ages, and it seems to work.
> 
> Hmm that gives me a lot of confidence ;-)  I'd hate to carry along bad
> assumptions while we try to make this all kosher... but I'm willing to
> defer to popular opinion on this one

yeah, I'm a bit wobbly about it.  Linus, what do you think?

> > I'd be a bit worried if any of these functions were returning pointers,
> > because one could certainly conceive of an arch+compiler combo which
> > returns pointers in a different register from integers (680x0?) but that's
> > not happening here.
> 
> Well, one is...
> 
> static long * return_EIO_ptr(void)
> {
> return ERR_PTR(-EIO);
> }
> ...
> static struct dentry *bad_inode_lookup(struct inode * dir,
> struct dentry *dentry, struct nameidata *nd)
> __attribute__((alias("return_EIO_ptr")));
> 
> Maybe it'd be better to lose the alias in this case then?  and go back
> to this:
> 
> static struct dentry *bad_inode_lookup(struct inode * dir,
> struct dentry *dentry, struct nameidata *nd)
> {
> return ERR_PTR(-EIO);
> }

A bit saner, but again, the old code used the same function for *everything*
and apart from the 32/64-bit thing, it worked.

Half a kb isn't much of course, but we've done lots of changes for a lot
less...


-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Andrew Morton wrote:
> On Thu, 04 Jan 2007 11:51:10 -0600
> Eric Sandeen <[EMAIL PROTECTED]> wrote:

>> Also - is it ok to alias a function with one signature to a function with
>> another signature?
> 
> Ordinarily I'd say no wucking fay, but that's effectively what we've been
> doing in there for ages, and it seems to work.

Hmm that gives me a lot of confidence ;-)  I'd hate to carry along bad
assumptions while we try to make this all kosher... but I'm willing to
defer to popular opinion on this one

> I'd be a bit worried if any of these functions were returning pointers,
> because one could certainly conceive of an arch+compiler combo which
> returns pointers in a different register from integers (680x0?) but that's
> not happening here.

Well, one is...

static long * return_EIO_ptr(void)
{
return ERR_PTR(-EIO);
}
...
static struct dentry *bad_inode_lookup(struct inode * dir,
struct dentry *dentry, struct nameidata *nd)
__attribute__((alias("return_EIO_ptr")));

Maybe it'd be better to lose the alias in this case then?  and go back
to this:

static struct dentry *bad_inode_lookup(struct inode * dir,
struct dentry *dentry, struct nameidata *nd)
{
return ERR_PTR(-EIO);
}

Thanks,
-Eric
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Andrew Morton]

On Thu, 04 Jan 2007 11:51:10 -0600
Eric Sandeen <[EMAIL PROTECTED]> wrote:

> Andrew Morton wrote:
> 
> > Al is correct, of course.  But the patch takes bad_inode.o from 280 up to 
> > 703
> > bytes, which is a bit sad for some cosmetic thing which nobody ever looks
> > at or modifies.
> >
> > Perhaps you can do
> >
> > static int return_EIO_int(void)
> > {
> > return -EIO;
> > }
> >
> > static int bad_file_release(struct inode * inode, struct file * filp)
> > __attribute__((alias("return_EIO_int")));
> > static int bad_file_fsync(struct inode * inode, struct file * filp)
> > __attribute__((alias("return_EIO_int")));
> >
> > etcetera?
> Ok, try this on for size.  Even though the gcc manual says alias doesn't work
> on all target machines, I assume linux arches are ok since alias is used
> in the core module init & exit code...
> 
> Also - is it ok to alias a function with one signature to a function with
> another signature?

Ordinarily I'd say no wucking fay, but that's effectively what we've been
doing in there for ages, and it seems to work.

I'd be a bit worried if any of these functions were returning pointers,
because one could certainly conceive of an arch+compiler combo which
returns pointers in a different register from integers (680x0?) but that's
not happening here.

> Note... I also realized that there are a couple of file ops which expect 
> unsigned
> returns... poll and get_unmapped_area.  The latter seems to be handled just 
> fine by
> the caller, which does IS_ERR gyrations to check for errnos.
> 
> I'm not so sure about poll; some callers put the return in a signed int, 
> others
> unsigned, not sure anyone is really checking for -EIO... I think this op 
> should
> probably be returning POLLERR, so that's what I've got in this version.

Yeah, that should all be OK.

-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Andrew Morton wrote:

> Al is correct, of course.  But the patch takes bad_inode.o from 280 up to 703
> bytes, which is a bit sad for some cosmetic thing which nobody ever looks
> at or modifies.
>
> Perhaps you can do
>
> static int return_EIO_int(void)
> {
>   return -EIO;
> }
>
> static int bad_file_release(struct inode * inode, struct file * filp)
>   __attribute__((alias("return_EIO_int")));
> static int bad_file_fsync(struct inode * inode, struct file * filp)
>   __attribute__((alias("return_EIO_int")));
>
> etcetera?
Ok, try this on for size.  Even though the gcc manual says alias doesn't work
on all target machines, I assume linux arches are ok since alias is used
in the core module init & exit code...

Also - is it ok to alias a function with one signature to a function with
another signature?

Note... I also realized that there are a couple of file ops which expect 
unsigned
returns... poll and get_unmapped_area.  The latter seems to be handled just 
fine by
the caller, which does IS_ERR gyrations to check for errnos.

I'm not so sure about poll; some callers put the return in a signed int, others
unsigned, not sure anyone is really checking for -EIO... I think this op should
probably be returning POLLERR, so that's what I've got in this version.

Anyway, here's the latest stab at this:

Signed-off-by: Eric Sandeen <[EMAIL PROTECTED]>

Index: linux-2.6.19/fs/bad_inode.c
===
--- linux-2.6.19.orig/fs/bad_inode.c
+++ linux-2.6.19/fs/bad_inode.c
@@ -14,59 +14,255 @@
 #include 
 #include 
 #include 
+#include 
 
-static int return_EIO(void)
+/* Generic EIO-returners, for different types of return values */
+
+static int return_EIO_int(void)
+{
+   return -EIO;
+}
+
+static ssize_t return_EIO_ssize(void)
+{
+   return -EIO;
+}
+
+static long return_EIO_long(void)
+{
+   return -EIO;
+}
+
+static loff_t return_EIO_loff(void)
 {
return -EIO;
 }
 
-#define EIO_ERROR ((void *) (return_EIO))
+static long * return_EIO_ptr(void)
+{
+   return ERR_PTR(-EIO);
+}
+
+/* All the specific file ops, aliased to something with the right retval */
+
+static loff_t bad_file_llseek(struct file *file, loff_t offset, int origin)
+   __attribute__((alias("return_EIO_loff")));
+
+static ssize_t bad_file_read(struct file *filp, char __user *buf,
+   size_t size, loff_t *ppos)
+   __attribute__((alias("return_EIO_ssize")));
+
+static ssize_t bad_file_write(struct file *filp, const char __user *buf,
+   size_t siz, loff_t *ppos)
+   __attribute__((alias("return_EIO_ssize")));
+
+static ssize_t bad_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
+   unsigned long nr_segs, loff_t pos)
+   __attribute__((alias("return_EIO_ssize")));
+
+static ssize_t bad_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
+   unsigned long nr_segs, loff_t pos)
+   __attribute__((alias("return_EIO_ssize")));
+
+static int bad_file_readdir(struct file * filp, void * dirent,
+   filldir_t filldir)
+   __attribute__((alias("return_EIO_int")));
+
+static unsigned int bad_file_poll(struct file *filp, poll_table *wait)
+{
+   return POLLERR;
+}
+
+static int bad_file_ioctl (struct inode * inode, struct file * filp,
+   unsigned int cmd, unsigned long arg)
+   __attribute__((alias("return_EIO_int")));
+
+static long bad_file_unlocked_ioctl(struct file *file, unsigned cmd,
+   unsigned long arg)
+   __attribute__((alias("return_EIO_long")));
+
+static long bad_file_compat_ioctl(struct file *file, unsigned int cmd,
+   unsigned long arg)
+   __attribute__((alias("return_EIO_long")));
+
+static int bad_file_mmap(struct file * file, struct vm_area_struct * vma)
+   __attribute__((alias("return_EIO_int")));
+
+static int bad_file_open(struct inode * inode, struct file * filp)
+   __attribute__((alias("return_EIO_int")));
+
+static int bad_file_flush(struct file *file, fl_owner_t id)
+   __attribute__((alias("return_EIO_int")));
+
+static int bad_file_release(struct inode * inode, struct file * filp)
+   __attribute__((alias("return_EIO_int")));
+
+static int bad_file_fsync(struct file * file, struct dentry *dentry,
+   int datasync)
+   __attribute__((alias("return_EIO_int")));
+
+static int bad_file_aio_fsync(struct kiocb *iocb, int datasync)
+   __attribute__((alias("return_EIO_int")));
+
+static int bad_file_fasync(int fd, struct file *filp, int on)
+   __attribute__((alias("return_EIO_int")));
+
+static int bad_file_lock(struct file *file, int cmd, struct file_lock *fl)
+   __attribute__((alias("return_EIO_int")));
+
+static ssize_t bad_file_sendfile(struct file *in_file, loff_t *ppos,
+   size_t count, read_actor_t actor, void *target)
+   

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Andrew Morton wrote:

 Al is correct, of course.  But the patch takes bad_inode.o from 280 up to 703
 bytes, which is a bit sad for some cosmetic thing which nobody ever looks
 at or modifies.

 Perhaps you can do

 static int return_EIO_int(void)
 {
   return -EIO;
 }

 static int bad_file_release(struct inode * inode, struct file * filp)
   __attribute__((alias(return_EIO_int)));
 static int bad_file_fsync(struct inode * inode, struct file * filp)
   __attribute__((alias(return_EIO_int)));

 etcetera?
Ok, try this on for size.  Even though the gcc manual says alias doesn't work
on all target machines, I assume linux arches are ok since alias is used
in the core module init  exit code...

Also - is it ok to alias a function with one signature to a function with
another signature?

Note... I also realized that there are a couple of file ops which expect 
unsigned
returns... poll and get_unmapped_area.  The latter seems to be handled just 
fine by
the caller, which does IS_ERR gyrations to check for errnos.

I'm not so sure about poll; some callers put the return in a signed int, others
unsigned, not sure anyone is really checking for -EIO... I think this op should
probably be returning POLLERR, so that's what I've got in this version.

Anyway, here's the latest stab at this:

Signed-off-by: Eric Sandeen [EMAIL PROTECTED]

Index: linux-2.6.19/fs/bad_inode.c
===
--- linux-2.6.19.orig/fs/bad_inode.c
+++ linux-2.6.19/fs/bad_inode.c
@@ -14,59 +14,255 @@
 #include linux/time.h
 #include linux/smp_lock.h
 #include linux/namei.h
+#include linux/poll.h
 
-static int return_EIO(void)
+/* Generic EIO-returners, for different types of return values */
+
+static int return_EIO_int(void)
+{
+   return -EIO;
+}
+
+static ssize_t return_EIO_ssize(void)
+{
+   return -EIO;
+}
+
+static long return_EIO_long(void)
+{
+   return -EIO;
+}
+
+static loff_t return_EIO_loff(void)
 {
return -EIO;
 }
 
-#define EIO_ERROR ((void *) (return_EIO))
+static long * return_EIO_ptr(void)
+{
+   return ERR_PTR(-EIO);
+}
+
+/* All the specific file ops, aliased to something with the right retval */
+
+static loff_t bad_file_llseek(struct file *file, loff_t offset, int origin)
+   __attribute__((alias(return_EIO_loff)));
+
+static ssize_t bad_file_read(struct file *filp, char __user *buf,
+   size_t size, loff_t *ppos)
+   __attribute__((alias(return_EIO_ssize)));
+
+static ssize_t bad_file_write(struct file *filp, const char __user *buf,
+   size_t siz, loff_t *ppos)
+   __attribute__((alias(return_EIO_ssize)));
+
+static ssize_t bad_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
+   unsigned long nr_segs, loff_t pos)
+   __attribute__((alias(return_EIO_ssize)));
+
+static ssize_t bad_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
+   unsigned long nr_segs, loff_t pos)
+   __attribute__((alias(return_EIO_ssize)));
+
+static int bad_file_readdir(struct file * filp, void * dirent,
+   filldir_t filldir)
+   __attribute__((alias(return_EIO_int)));
+
+static unsigned int bad_file_poll(struct file *filp, poll_table *wait)
+{
+   return POLLERR;
+}
+
+static int bad_file_ioctl (struct inode * inode, struct file * filp,
+   unsigned int cmd, unsigned long arg)
+   __attribute__((alias(return_EIO_int)));
+
+static long bad_file_unlocked_ioctl(struct file *file, unsigned cmd,
+   unsigned long arg)
+   __attribute__((alias(return_EIO_long)));
+
+static long bad_file_compat_ioctl(struct file *file, unsigned int cmd,
+   unsigned long arg)
+   __attribute__((alias(return_EIO_long)));
+
+static int bad_file_mmap(struct file * file, struct vm_area_struct * vma)
+   __attribute__((alias(return_EIO_int)));
+
+static int bad_file_open(struct inode * inode, struct file * filp)
+   __attribute__((alias(return_EIO_int)));
+
+static int bad_file_flush(struct file *file, fl_owner_t id)
+   __attribute__((alias(return_EIO_int)));
+
+static int bad_file_release(struct inode * inode, struct file * filp)
+   __attribute__((alias(return_EIO_int)));
+
+static int bad_file_fsync(struct file * file, struct dentry *dentry,
+   int datasync)
+   __attribute__((alias(return_EIO_int)));
+
+static int bad_file_aio_fsync(struct kiocb *iocb, int datasync)
+   __attribute__((alias(return_EIO_int)));
+
+static int bad_file_fasync(int fd, struct file *filp, int on)
+   __attribute__((alias(return_EIO_int)));
+
+static int bad_file_lock(struct file *file, int cmd, struct file_lock *fl)
+   __attribute__((alias(return_EIO_int)));
+
+static ssize_t bad_file_sendfile(struct file *in_file, loff_t *ppos,
+   size_t count, read_actor_t actor, void *target)
+   

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Andrew Morton]

On Thu, 04 Jan 2007 11:51:10 -0600
Eric Sandeen [EMAIL PROTECTED] wrote:

 Andrew Morton wrote:
 
  Al is correct, of course.  But the patch takes bad_inode.o from 280 up to 
  703
  bytes, which is a bit sad for some cosmetic thing which nobody ever looks
  at or modifies.
 
  Perhaps you can do
 
  static int return_EIO_int(void)
  {
  return -EIO;
  }
 
  static int bad_file_release(struct inode * inode, struct file * filp)
  __attribute__((alias(return_EIO_int)));
  static int bad_file_fsync(struct inode * inode, struct file * filp)
  __attribute__((alias(return_EIO_int)));
 
  etcetera?
 Ok, try this on for size.  Even though the gcc manual says alias doesn't work
 on all target machines, I assume linux arches are ok since alias is used
 in the core module init  exit code...
 
 Also - is it ok to alias a function with one signature to a function with
 another signature?

Ordinarily I'd say no wucking fay, but that's effectively what we've been
doing in there for ages, and it seems to work.

I'd be a bit worried if any of these functions were returning pointers,
because one could certainly conceive of an arch+compiler combo which
returns pointers in a different register from integers (680x0?) but that's
not happening here.

 Note... I also realized that there are a couple of file ops which expect 
 unsigned
 returns... poll and get_unmapped_area.  The latter seems to be handled just 
 fine by
 the caller, which does IS_ERR gyrations to check for errnos.
 
 I'm not so sure about poll; some callers put the return in a signed int, 
 others
 unsigned, not sure anyone is really checking for -EIO... I think this op 
 should
 probably be returning POLLERR, so that's what I've got in this version.

Yeah, that should all be OK.

-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Andrew Morton wrote:
 On Thu, 04 Jan 2007 11:51:10 -0600
 Eric Sandeen [EMAIL PROTECTED] wrote:

 Also - is it ok to alias a function with one signature to a function with
 another signature?
 
 Ordinarily I'd say no wucking fay, but that's effectively what we've been
 doing in there for ages, and it seems to work.

Hmm that gives me a lot of confidence ;-)  I'd hate to carry along bad
assumptions while we try to make this all kosher... but I'm willing to
defer to popular opinion on this one

 I'd be a bit worried if any of these functions were returning pointers,
 because one could certainly conceive of an arch+compiler combo which
 returns pointers in a different register from integers (680x0?) but that's
 not happening here.

Well, one is...

static long * return_EIO_ptr(void)
{
return ERR_PTR(-EIO);
}
...
static struct dentry *bad_inode_lookup(struct inode * dir,
struct dentry *dentry, struct nameidata *nd)
__attribute__((alias(return_EIO_ptr)));

Maybe it'd be better to lose the alias in this case then?  and go back
to this:

static struct dentry *bad_inode_lookup(struct inode * dir,
struct dentry *dentry, struct nameidata *nd)
{
return ERR_PTR(-EIO);
}

Thanks,
-Eric
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Andrew Morton]

On Thu, 04 Jan 2007 12:33:59 -0600
Eric Sandeen [EMAIL PROTECTED] wrote:

 Andrew Morton wrote:
  On Thu, 04 Jan 2007 11:51:10 -0600
  Eric Sandeen [EMAIL PROTECTED] wrote:
 
  Also - is it ok to alias a function with one signature to a function with
  another signature?
  
  Ordinarily I'd say no wucking fay, but that's effectively what we've been
  doing in there for ages, and it seems to work.
 
 Hmm that gives me a lot of confidence ;-)  I'd hate to carry along bad
 assumptions while we try to make this all kosher... but I'm willing to
 defer to popular opinion on this one

yeah, I'm a bit wobbly about it.  Linus, what do you think?

  I'd be a bit worried if any of these functions were returning pointers,
  because one could certainly conceive of an arch+compiler combo which
  returns pointers in a different register from integers (680x0?) but that's
  not happening here.
 
 Well, one is...
 
 static long * return_EIO_ptr(void)
 {
 return ERR_PTR(-EIO);
 }
 ...
 static struct dentry *bad_inode_lookup(struct inode * dir,
 struct dentry *dentry, struct nameidata *nd)
 __attribute__((alias(return_EIO_ptr)));
 
 Maybe it'd be better to lose the alias in this case then?  and go back
 to this:
 
 static struct dentry *bad_inode_lookup(struct inode * dir,
 struct dentry *dentry, struct nameidata *nd)
 {
 return ERR_PTR(-EIO);
 }

A bit saner, but again, the old code used the same function for *everything*
and apart from the 32/64-bit thing, it worked.

Half a kb isn't much of course, but we've done lots of changes for a lot
less...


-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Linus Torvalds]

On Thu, 4 Jan 2007, Andrew Morton wrote:

 On Thu, 04 Jan 2007 12:33:59 -0600
 Eric Sandeen [EMAIL PROTECTED] wrote:
 
  Andrew Morton wrote:
   On Thu, 04 Jan 2007 11:51:10 -0600
   Eric Sandeen [EMAIL PROTECTED] wrote:
  
   Also - is it ok to alias a function with one signature to a function with
   another signature?
   
   Ordinarily I'd say no wucking fay, but that's effectively what we've been
   doing in there for ages, and it seems to work.
  
  Hmm that gives me a lot of confidence ;-)  I'd hate to carry along bad
  assumptions while we try to make this all kosher... but I'm willing to
  defer to popular opinion on this one
 
 yeah, I'm a bit wobbly about it.  Linus, what do you think?

I don't much care. If we ever find an architecture where it matters, we'll 
unalias them. In the meantime, we've for the longest time just known that 
calling conventions don't care about argument types on all the 
architectures we've been on, so we've aliased things to the same function.

But it's not very common, so we can stop doing it. 

But I'd argue we should only do it if there is an actual 
honest-to-goodness reason to do so. Usually it only matters for

 - return types are fundamentally different (FP vs integer vs pointer)

 - calling convention has callee popping the arguments (normal in Pascal, 
   very unusual in C, because it also breaks lots of historical code, 
   and is simply not workable with KR C, where perfectly normal things 
   like open() take either two or three arguments without being 
   varargs).

In general, this just isn't an issue for the kernel. Other systems have 
had basically NO typing what-so-ever for functions, and use aliasing much 
more extensively. We only do it for a few ratehr rare things.

Linus
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Al Viro]

On Thu, Jan 04, 2007 at 11:09:31AM -0800, Linus Torvalds wrote:
 
 But I'd argue we should only do it if there is an actual 
 honest-to-goodness reason to do so.

How about makes call graph analysis easier? ;-)  In principle, I have
no problem with force-casting, but it'd better be cast to the right
type...

(And yes, there's a bunch of sparse-based fun in making dealing with
call graph analysis and sane annotations needed for that).
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Al Viro]

On Thu, Jan 04, 2007 at 07:14:51PM +, Al Viro wrote:
 On Thu, Jan 04, 2007 at 11:09:31AM -0800, Linus Torvalds wrote:
  
  But I'd argue we should only do it if there is an actual 
  honest-to-goodness reason to do so.
 
 How about makes call graph analysis easier? ;-)  In principle, I have
 no problem with force-casting, but it'd better be cast to the right
 type...
 
 (And yes, there's a bunch of sparse-based fun in making dealing with
 call graph analysis and sane annotations needed for that).

PS: what would be the sane strategy for timer series merge, BTW?  It
touches a whole lot of files in rather trivial ways (see below for current
stat), but it's gradually mergable and after the first 4 chunks the rest
can go in independently (per-driver, if we want to go for insane length,
but most of those will be absolutely trivial and I'd rather lump them
into bigger groups).  And those 4 chunks in the beginning of series are
safe to merge at any point - result in guaranteed to be identical code...

 arch/alpha/kernel/srmcons.c|8 +--
 arch/arm/common/sharpsl_pm.c   |   10 ++--
 arch/arm/mach-iop32x/n2100.c   |5 +-
 arch/arm/mach-pxa/lubbock.c|   12 ++---
 arch/i386/kernel/time.c|6 +-
 arch/i386/kernel/tsc.c |5 +-
 arch/i386/mach-voyager/voyager_thread.c|5 +-
 arch/ia64/kernel/mca.c |   12 ++---
 arch/ia64/kernel/salinfo.c |5 +-
 arch/ia64/sn/kernel/bte.c  |7 +--
 arch/ia64/sn/kernel/bte_error.c|   17 ++
 arch/ia64/sn/kernel/huberror.c |2 -
 arch/ia64/sn/kernel/mca.c  |5 +-
 arch/ia64/sn/kernel/xpc_channel.c  |4 --
 arch/ia64/sn/kernel/xpc_main.c |   18 ++-
 arch/mips/lasat/picvue_proc.c  |5 +-
 arch/mips/sgi-ip22/ip22-reset.c|   20 +++-
 arch/mips/sgi-ip32/ip32-reset.c|   10 ++--
 arch/powerpc/oprofile/op_model_cell.c  |6 +-
 arch/powerpc/platforms/chrp/chrp.h |2 -
 arch/powerpc/platforms/chrp/setup.c|4 +-
 arch/powerpc/platforms/powermac/low_i2c.c  |7 +--
 arch/ppc/syslib/m8xx_wdt.c |   10 ++--
 arch/s390/mm/cmm.c |   23 +++--
 arch/sh/drivers/push-switch.c  |9 +--
 arch/um/drivers/net_kern.c |7 +--
 arch/x86_64/kernel/pci-calgary.c   |7 +--
 arch/xtensa/platform-iss/console.c |   10 +---
 arch/xtensa/platform-iss/network.c |   13 ++---
 block/as-iosched.c |7 +--
 block/cfq-iosched.c|   15 ++
 block/ll_rw_blk.c  |9 +--
 drivers/acpi/sbs.c |7 +--
 drivers/acpi/thermal.c |   26 +++---
 drivers/atm/ambassador.c   |   10 +---
 drivers/atm/firestream.c   |   10 +---
 drivers/atm/horizon.c  |   11 +---
 drivers/atm/idt77252.c |   14 ++---
 drivers/atm/lanai.c|7 +--
 drivers/atm/nicstar.c  |8 +--
 drivers/atm/suni.c |8 +--
 drivers/base/firmware_class.c  |   11 
 drivers/block/DAC960.c |8 +--
 drivers/block/DAC960.h |2 -
 drivers/block/cpqarray.c   |   10 +---
 drivers/block/swim3.c  |   43 +---
 drivers/block/ub.c |   22 ++--
 drivers/block/umem.c   |5 +-
 drivers/block/xd.c |4 +-
 drivers/block/xd.h |2 -
 drivers/bluetooth/bluecard_cs.c|7 +--
 drivers/bluetooth/hci_bcsp.c   |7 +--
 drivers/cdrom/aztcd.c  |   10 ++--
 drivers/cdrom/cdu31a.c |5 +-
 drivers/cdrom/cm206.c  |   11 ++--
 drivers/char/drm/via_dmablit.c |7 +--
 drivers/char/dtlk.c|7 +--
 drivers/char/epca.c|7 +--
 drivers/char/genrtc.c  |7 +--
 drivers/char/ip2/i2ellis.c |8 +--
 drivers/char/ip2/i2lib.c   |9 +--
 drivers/char/ipmi/ipmi_msghandler.c|4 +-
 drivers/char/ipmi/ipmi_si_intf.c   |5 +-
 drivers/char/moxa.c|   21 +++-
 drivers/char/mspec.c   |2 -
 drivers/char/n_r3964.c |9 +--
 drivers/char/nwbutton.c  

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Mikael Pettersson]

On Thu, 04 Jan 2007 11:51:10 -0600, Eric Sandeen wrote:
Also - is it ok to alias a function with one signature to a function with
another signature?

NO!

Specific cases of type mismatches may work on many machines
(say different pointer types as long as they aren't accessed),
but in general this won't work since types affect calling conventions.
Abusing aliasing like this is exactly like casting a function
pointer to a different type that it actually has, and then invoking
it at that type: all bets are off.

Don't do this. Ever.

/Mikael
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Linus Torvalds]

On Thu, 4 Jan 2007, Al Viro wrote:
 
 How about makes call graph analysis easier? ;-)  In principle, I have
 no problem with force-casting, but it'd better be cast to the right
 type...

Do we really care in the kernel? We simply never use function pointer 
casts like this for anything non-trivial, so if the graph analysis just 
doesn't work for those cases, do we really even care?

The only case I can _remember_ us doing this for is literally the 
error-returning functions, where the call graph finding them really 
doesn't matter, I think.

So I don't _object_ to that reason, I just wonder whether it's really a 
big issue..

Linus
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Linus Torvalds]

On Thu, 4 Jan 2007, Al Viro wrote:
 
 PS: what would be the sane strategy for timer series merge, BTW?

I think Andrew may care more. I just care about it happening after 2.6.20.

Whether we can do it really early after that, or whether we should do it 
as the last thing just before releasing -rc1 (to avoid having other people 
have to fix up conflicts during the merge window), who knows..

Linus
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Al Viro]

On Thu, Jan 04, 2007 at 11:30:22AM -0800, Linus Torvalds wrote:
 
 
 On Thu, 4 Jan 2007, Al Viro wrote:
  
  How about makes call graph analysis easier? ;-)  In principle, I have
  no problem with force-casting, but it'd better be cast to the right
  type...
 
 Do we really care in the kernel? We simply never use function pointer 
 casts like this for anything non-trivial, so if the graph analysis just 
 doesn't work for those cases, do we really even care?

Umm...  Let me put it that way - amount of things that can be done to
void * is much more than what can be done to function pointers.  So
keeping track of them gets easier if we never do casts to/from void *.
What's more, very few places in the kernel try to do that _and_ most
of those that do are simply too lazy to declare local variable with
the right type.  bad_inode.c covers most of what remains.

IMO we ought to start checking for that kind of stuff; note that we _still_
have strugglers from pt_regs removal where interrupt handler still takes
3 arguments, but we don't notice since argument of request_irq() is cast
to void * ;-/

That's local stuff; however, when trying to do non-local work (e.g. deduce
that foo() may be called from BH, bar() is always called from process
context, etc. _without_ fuckloads of annotations all over the place), the
ban on mixing void * with function pointers helps a _lot_.

So my main issue with fs/bad_inode.c is not even cast per se; it's that
cast is to void *.
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Andrew Morton]

On Thu, 4 Jan 2007 20:24:12 +
Al Viro [EMAIL PROTECTED] wrote:

 So my main issue with fs/bad_inode.c is not even cast per se; it's that
 cast is to void *.

But Eric's latest patch is OK in that regard, isn't it?  It might confuse
parsers (in fixable ways), but it is type-correct and has no casts.  (Well,
it kinda has an link-time cast).
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Andrew Morton wrote:
 On Thu, 4 Jan 2007 20:24:12 +
 Al Viro [EMAIL PROTECTED] wrote:
 
 So my main issue with fs/bad_inode.c is not even cast per se; it's that
 cast is to void *.
 
 But Eric's latest patch is OK in that regard, isn't it?  It might confuse
 parsers (in fixable ways), but it is type-correct and has no casts.  (Well,
 it kinda has an link-time cast).

Even if it is, I'm starting to wonder if all this tricksiness is really
worth it for 400 bytes or so.  :)

-Eric
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Andrew Morton]

On Thu, 04 Jan 2007 15:04:17 -0600
Eric Sandeen [EMAIL PROTECTED] wrote:

 Andrew Morton wrote:
  On Thu, 4 Jan 2007 20:24:12 +
  Al Viro [EMAIL PROTECTED] wrote:
  
  So my main issue with fs/bad_inode.c is not even cast per se; it's that
  cast is to void *.
  
  But Eric's latest patch is OK in that regard, isn't it?  It might confuse
  parsers (in fixable ways), but it is type-correct and has no casts.  (Well,
  it kinda has an link-time cast).
 
 Even if it is, I'm starting to wonder if all this tricksiness is really
 worth it for 400 bytes or so.  :)
 

Ah, but it's a learning opportunity!

btw, couldn't we fix this bug with a simple old

--- a/fs/bad_inode.c~a
+++ a/fs/bad_inode.c
@@ -15,7 +15,7 @@
 #include linux/smp_lock.h
 #include linux/namei.h
 
-static int return_EIO(void)
+static long return_EIO(void)
 {
return -EIO;
 }
_

?
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Andrew Morton wrote:
 On Thu, 04 Jan 2007 15:04:17 -0600
 Eric Sandeen [EMAIL PROTECTED] wrote:
 
 Andrew Morton wrote:
 On Thu, 4 Jan 2007 20:24:12 +
 Al Viro [EMAIL PROTECTED] wrote:

 So my main issue with fs/bad_inode.c is not even cast per se; it's that
 cast is to void *.
 But Eric's latest patch is OK in that regard, isn't it?  It might confuse
 parsers (in fixable ways), but it is type-correct and has no casts.  (Well,
 it kinda has an link-time cast).
 Even if it is, I'm starting to wonder if all this tricksiness is really
 worth it for 400 bytes or so.  :)

 
 Ah, but it's a learning opportunity!

*grin*

 btw, couldn't we fix this bug with a simple old
 
 --- a/fs/bad_inode.c~a
 +++ a/fs/bad_inode.c
 @@ -15,7 +15,7 @@
  #include linux/smp_lock.h
  #include linux/namei.h
  
 -static int return_EIO(void)
 +static long return_EIO(void)
  {
   return -EIO;
  }
 _
 
 ?

What about ops that return loff_t (64 bits) on 32-bit arches and stuff
it into 2 registers

#include stdio.h
#include sys/types.h
#include sys/errno.h

static long return_EIO(void)
{
return -EIO;
}

#define EIO_ERROR ((void *) (return_EIO))

int main(int argc, char ** argv)
{
loff_t error;
loff_t (*fn_ptr) (int);

fn_ptr = EIO_ERROR;

error = fn_ptr(0);
printf(and... error is %lld\n, error);
return 0;
}

[root]# ./ssize_eio
and... error is 8589934587
[root]# uname -m
i686

I'm still not convinced that this is the best place to be clever :)

-Eric
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Linus Torvalds]

On Thu, 4 Jan 2007, Eric Sandeen wrote:
 Andrew Morton wrote:
 
  btw, couldn't we fix this bug with a simple old
  
  --- a/fs/bad_inode.c~a
  +++ a/fs/bad_inode.c
  @@ -15,7 +15,7 @@
   #include linux/smp_lock.h
   #include linux/namei.h
   
  -static int return_EIO(void)
  +static long return_EIO(void)
   {
  return -EIO;
   }
  _
  
  ?
 
 What about ops that return loff_t (64 bits) on 32-bit arches and stuff
 it into 2 registers

Do we actually have cases where we cast to a different return value?

I'll happily cast away arguments that aren't used, but I'm not sure that 
we ever should cast different return values (not int vs long, but also 
not loff_t etc). 

On 32-bit architectures, 64-bit entities may be returned totally different 
ways (ie things like caller allocates space for them and passes in a 
magic pointer to the return value as the first _real_ argument).

So with my previous email, I was definitely _not_ trying to say that 
casting function pointers is ok. In practice it is ok when the _arguments_ 
differ, but not necessarily when the _return-type_ differs.

I was cc'd into the discussion late, so I didn't realize that we 
apparently already have a situation where changing the return value to 
long might make a difference. If so, I agree that we shouldn't do this 
at all (although Andrew's change to long seems perfectly fine as a make 
old cases continue to work patch if it actually matters).

Linus
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Linus Torvalds wrote:
 
 On Thu, 4 Jan 2007, Eric Sandeen wrote:
 Andrew Morton wrote:

 btw, couldn't we fix this bug with a simple old

 --- a/fs/bad_inode.c~a
 +++ a/fs/bad_inode.c
 @@ -15,7 +15,7 @@
  #include linux/smp_lock.h
  #include linux/namei.h
  
 -static int return_EIO(void)
 +static long return_EIO(void)
  {
 return -EIO;
  }
 _

 ?
 What about ops that return loff_t (64 bits) on 32-bit arches and stuff
 it into 2 registers
 
 Do we actually have cases where we cast to a different return value?

Today, via the void * function casts in the bad file/inode ops, in
effect yes.

static int return_EIO(void)
{
return -EIO;
}

#define EIO_ERROR ((void *) (return_EIO))

...
.listxattr  = EIO_ERROR,

but listxattr is supposed to return a ssize_t, which is 64 bits on some
platforms, and only 32 bits get filled in thanks to the (void *) cast.
So we wind up with something other than the return value we expect...

Andrew's long suggestion breaks things the other way, with 64-bit
returning ops on 32-bit arches which again only pick up the first 32
bits thanks to the (void *) cast.

If we're really happy with casting away the function arguments (which
are not -used- in the bad_foo ops anyway), then I'd maybe suggest going
back to my first try at this thing:

static int return_EIO_int(void)
{
return -EIO;
}
#define EIO_ERROR_INT ((void *) (return_EIO_int))

static struct inode_operations bad_inode_ops =
{
.create = EIO_ERROR_INT,
...etc...

which is most like what we have today, except with specific return types.

-Eric
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Al Viro]

On Thu, Jan 04, 2007 at 01:30:47PM -0800, Linus Torvalds wrote:
 
 I'll happily cast away arguments that aren't used, but I'm not sure that 
 we ever should cast different return values (not int vs long, but also 
 not loff_t etc). 
 
 On 32-bit architectures, 64-bit entities may be returned totally different 
 ways (ie things like caller allocates space for them and passes in a 
 magic pointer to the return value as the first _real_ argument).
 
 So with my previous email, I was definitely _not_ trying to say that 
 casting function pointers is ok. In practice it is ok when the _arguments_ 
 differ, but not necessarily when the _return-type_ differs.
 
 I was cc'd into the discussion late, so I didn't realize that we 
 apparently already have a situation where changing the return value to 
 long might make a difference. If so, I agree that we shouldn't do this 
 at all (although Andrew's change to long seems perfectly fine as a make 
 old cases continue to work patch if it actually matters).

We do.
loff_t (*llseek) (struct file *, loff_t, int);
...
int (*readdir) (struct file *, void *, filldir_t);

static const struct file_operations bad_file_ops =
{
.llseek = EIO_ERROR,
...
.readdir= EIO_ERROR,


Moreover, we have int, loff_t, ssize_t and long, plus the unsigned variants.
At least 3 versions, unless you want to mess with ifdefs to reduce them to
two.
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Mitchell Blank Jr]

Al Viro wrote:
 At least 3 versions, unless you want to mess with ifdefs to reduce them to
 two.

I don't think you need to do fancy #ifdef's:

static s32 return_eio_32(void) { return -EIO; }
static s64 return_eio_64(void) { return -EIO; }
extern void return_eio_unknown(void);   /* Doesn't exist */
#define return_eio(type)((sizeof(type) == 4)\
? ((void *) return_eio_32)  \
: ((sizeof(type) == 8)  \
? ((void *) return_eio_64)  \
: ((void *) return_eio_unknown)))

-Mitch
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Linus Torvalds]

On Thu, 4 Jan 2007, Mitchell Blank Jr wrote:
 
 I don't think you need to do fancy #ifdef's:
 
 static s32 return_eio_32(void) { return -EIO; }
 static s64 return_eio_64(void) { return -EIO; }
 extern void return_eio_unknown(void);   /* Doesn't exist */
 #define return_eio(type)((sizeof(type) == 4)  \
   ? ((void *) return_eio_32)  \
   : ((sizeof(type) == 8)  \
   ? ((void *) return_eio_64)  \
   : ((void *) return_eio_unknown)))

Well, that probably would work, but it's also true that returning a 64-bit 
value on a 32-bit platform really _does_ depend on more than the size.

For an example of this, try compiling this:

long long a(void)
{
return -1;
}

struct dummy { int a, b };

struct dummy b(void)
{
struct dummy retval = { -1 , -1 };
return retval;
}

on x86.

Now, I don't think we actually have anything like this in the kernel, and 
your example is likely to work very well in practice, but once we start 
doing tricks like this, I actually think it's getting easier to just say 
let's not play tricks with function types at all.

Anybody want to send in the patch that just generates separate versions 
for

loff_t eio_llseek(struct file *file, loff_t offset, int origin)
{
return -EIO;
}

int eio_readdir(struct file *filp, void *dirent, filldir_t filldir)
{
return -EIO;
..

and so on?

Linus
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Linus Torvalds wrote:
 Anybody want to send in the patch that just generates separate versions 
 for
 
   loff_t eio_llseek(struct file *file, loff_t offset, int origin)
   {
   return -EIO;
   }
 
   int eio_readdir(struct file *filp, void *dirent, filldir_t filldir)
   {
   return -EIO;
   ..
 
 and so on?

That's more or less what I sent at http://lkml.org/lkml/2007/1/3/244

+static int bad_file_readdir(struct file * filp, void * dirent,
+   filldir_t filldir)
+{
+   return -EIO;
+}

... etc

Thanks,
-Eric
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Mitchell Blank Jr]

Linus Torvalds wrote:
 Well, that probably would work, but it's also true that returning a 64-bit 
 value on a 32-bit platform really _does_ depend on more than the size.

Yeah, obviously this is restricted to the signed-integer case.  My point
was just that you could have the compiler figure out which variant to pick
for loff_t automatically.

 let's not play tricks with function types at all.

I think I agree.  The real (but harder) fix for the wasted space issue
would be to get the toolchain to automatically combine functions that
end up compiling into identical assembly.

-Mitch
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Andrew Morton]

On Thu, 4 Jan 2007 14:35:09 -0800 (PST)
Linus Torvalds [EMAIL PROTECTED] wrote:

 Anybody want to send in the patch that just generates separate versions 
 for
 
   loff_t eio_llseek(struct file *file, loff_t offset, int origin)
   {
   return -EIO;
   }
 
   int eio_readdir(struct file *filp, void *dirent, filldir_t filldir)
   {
   return -EIO;
   ..
 

That's what I currently have queued.  It increases bad_inode.o text from 
200-odd bytes to 700-odd :(

looks at sys_ni_syscall
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Linus Torvalds]

On Thu, 4 Jan 2007, Andrew Morton wrote:
 
 That's what I currently have queued.  It increases bad_inode.o text from 
 200-odd bytes to 700-odd :(

Then I think we're ok. We do care about bytes, but we care more about 
bytes that actually ever hit the icache or dcache, and this will 
effectively do neither.

 looks at sys_ni_syscall

That one should be ok. System calls by definition have the same return 
type, since they all have the same call site. So that one really does fit 
the argument types differ, but the return type is the same case. 

Linus
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Linus Torvalds wrote:

 On Thu, 4 Jan 2007, Andrew Morton wrote:
   
 That's what I currently have queued.  It increases bad_inode.o text from 
 200-odd bytes to 700-odd :(
 
 Then I think we're ok. We do care about bytes, but we care more about 
 bytes that actually ever hit the icache or dcache, and this will 
 effectively do neither.

   
Oh good.  Resolution?  ;-)

Andrew, if you stick with what you've got, you might want this on top of it.

Mostly cosmetic, making placement of * for pointers consistently  *foo
not * foo, (was a mishmash before, from cut-n-paste), but also making 
.poll return POLLERR.

Thanks,
-Eric

Signed-off-by: Eric Sandeen [EMAIL PROTECTED]

Index: linux-2.6.19/fs/bad_inode.c
===
--- linux-2.6.19.orig/fs/bad_inode.c
+++ linux-2.6.19/fs/bad_inode.c
@@ -46,18 +46,17 @@ static ssize_t bad_file_aio_write(struct
return -EIO;
 }
 
-static int bad_file_readdir(struct file * filp, void * dirent,
-   filldir_t filldir)
+static int bad_file_readdir(struct file *filp, void *dirent, filldir_t filldir)
 {
return -EIO;
 }
 
 static unsigned int bad_file_poll(struct file *filp, poll_table *wait)
 {
-   return -EIO;
+   return POLLERR;
 }
 
-static int bad_file_ioctl (struct inode * inode, struct file * filp,
+static int bad_file_ioctl (struct inode *inode, struct file *filp,
unsigned int cmd, unsigned long arg)
 {
return -EIO;
@@ -75,12 +74,12 @@ static long bad_file_compat_ioctl(struct
return -EIO;
 }
 
-static int bad_file_mmap(struct file * file, struct vm_area_struct * vma)
+static int bad_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
return -EIO;
 }
 
-static int bad_file_open(struct inode * inode, struct file * filp)
+static int bad_file_open(struct inode *inode, struct file *filp)
 {
return -EIO;
 }
@@ -90,12 +89,12 @@ static int bad_file_flush(struct file *f
return -EIO;
 }
 
-static int bad_file_release(struct inode * inode, struct file * filp)
+static int bad_file_release(struct inode *inode, struct file *filp)
 {
return -EIO;
 }
 
-static int bad_file_fsync(struct file * file, struct dentry *dentry,
+static int bad_file_fsync(struct file *file, struct dentry *dentry,
int datasync)
 {
return -EIO;
@@ -140,7 +139,7 @@ static int bad_file_check_flags(int flag
return -EIO;
 }
 
-static int bad_file_dir_notify(struct file * file, unsigned long arg)
+static int bad_file_dir_notify(struct file *file, unsigned long arg)
 {
return -EIO;
 }
@@ -194,54 +193,54 @@ static const struct file_operations bad_
.splice_read= bad_file_splice_read,
 };
 
-static int bad_inode_create (struct inode * dir, struct dentry * dentry,
+static int bad_inode_create (struct inode *dir, struct dentry *dentry,
int mode, struct nameidata *nd)
 {
return -EIO;
 }
 
-static struct dentry *bad_inode_lookup(struct inode * dir,
+static struct dentry *bad_inode_lookup(struct inode *dir,
struct dentry *dentry, struct nameidata *nd)
 {
return ERR_PTR(-EIO);
 }
 
-static int bad_inode_link (struct dentry * old_dentry, struct inode * dir,
+static int bad_inode_link (struct dentry *old_dentry, struct inode *dir,
struct dentry *dentry)
 {
return -EIO;
 }
 
-static int bad_inode_unlink(struct inode * dir, struct dentry *dentry)
+static int bad_inode_unlink(struct inode *dir, struct dentry *dentry)
 {
return -EIO;
 }
 
-static int bad_inode_symlink (struct inode * dir, struct dentry *dentry,
-   const char * symname)
+static int bad_inode_symlink (struct inode *dir, struct dentry *dentry,
+   const char *symname)
 {
return -EIO;
 }
 
-static int bad_inode_mkdir(struct inode * dir, struct dentry * dentry,
+static int bad_inode_mkdir(struct inode *dir, struct dentry *dentry,
int mode)
 {
return -EIO;
 }
 
-static int bad_inode_rmdir (struct inode * dir, struct dentry *dentry)
+static int bad_inode_rmdir (struct inode *dir, struct dentry *dentry)
 {
return -EIO;
 }
 
-static int bad_inode_mknod (struct inode * dir, struct dentry *dentry,
+static int bad_inode_mknod (struct inode *dir, struct dentry *dentry,
int mode, dev_t rdev)
 {
return -EIO;
 }
 
-static int bad_inode_rename (struct inode * old_dir, struct dentry *old_dentry,
-   struct inode * new_dir, struct dentry *new_dentry)
+static int bad_inode_rename (struct inode *old_dir, struct dentry *old_dentry,
+   struct inode *new_dir, struct dentry *new_dentry)
 {
return -EIO;
 }
@@ -337,7 +336,7 @@ static struct inode_operations bad_inode
  * on it to fail from this point on.
  */
  
-void make_bad_inode(struct inode * inode) 
+void make_bad_inode(struct inode *inode) 
 {

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Al Viro]

On Thu, Jan 04, 2007 at 03:21:06PM -0800, Mitchell Blank Jr wrote:
 Linus Torvalds wrote:
  Well, that probably would work, but it's also true that returning a 64-bit 
  value on a 32-bit platform really _does_ depend on more than the size.
 
 Yeah, obviously this is restricted to the signed-integer case.  My point
 was just that you could have the compiler figure out which variant to pick
 for loff_t automatically.
 
  let's not play tricks with function types at all.
 
 I think I agree.  The real (but harder) fix for the wasted space issue
 would be to get the toolchain to automatically combine functions that
 end up compiling into identical assembly.

Can't do.

int f(void)
{
return 0;
}

int g(void)
{
return 0;
}

int is_f(int (*p)(void))
{
return p == f;
}

main()
{
printf(%d %d\n, is_f(f), is_f(g));
}

would better produce
1 0
for anything resembling a sane C compiler.  Comparing pointers to
functions for equality is a well-defined operation and it's not
to be messed with.

You _can_ compile g into jump to f, but that's it.  And that, AFAICS,
is what gcc does.
-
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/

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Andrew Morton]

On Wed, 03 Jan 2007 17:46:00 -0600
Eric Sandeen <[EMAIL PROTECTED]> wrote:

> Take 2... all in one file.  I suppose I really did know better than 
> to create that new header.   ;-) 
> 
> Better?
> 
> ---
> 
> CVE-2006-5753 is for a case where an inode can be marked bad, switching 
> the ops to bad_inode_ops, which are all connected as:
> 
> static int return_EIO(void)
> {
> return -EIO;
> }
> 
> #define EIO_ERROR ((void *) (return_EIO))
> 
> static struct inode_operations bad_inode_ops =
> {
> .create = bad_inode_create
> ...etc...
> 
> The problem here is that the void cast causes return types to not be 
> promoted, and for ops such as listxattr which expect more than 32 bits of
> return value, the 32-bit -EIO is interpreted as a large positive 64-bit 
> number, i.e. 0xfffa instead of 0xfffa.
> 
> This goes particularly badly when the return value is taken as a number of
> bytes to copy into, say, a user's buffer for example...
> 
> I originally had coded up the fix by creating a return_EIO_ macro
> for each return type, like this:
> 
> static int return_EIO_int(void)
> {
>   return -EIO;
> }
> #define EIO_ERROR_INT ((void *) (return_EIO_int))
> 
> static struct inode_operations bad_inode_ops =
> {
>   .create = EIO_ERROR_INT,
> ...etc...
> 
> but Al felt that it was probably better to create an EIO-returner for each 
> actual op signature.  Since so few ops share a signature, I just went ahead 
> & created an EIO function for each individual file & inode op that returns
> a value.
> 

Al is correct, of course.  But the patch takes bad_inode.o from 280 up to 703
bytes, which is a bit sad for some cosmetic thing which nobody ever looks
at or modifies.

Perhaps you can do

static int return_EIO_int(void)
{
return -EIO;
}

static int bad_file_release(struct inode * inode, struct file * filp)
__attribute__((alias("return_EIO_int")));
static int bad_file_fsync(struct inode * inode, struct file * filp)
__attribute__((alias("return_EIO_int")));

etcetera?
-
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/

[UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Take 2... all in one file.  I suppose I really did know better than 
to create that new header.   ;-) 

Better?

---

CVE-2006-5753 is for a case where an inode can be marked bad, switching 
the ops to bad_inode_ops, which are all connected as:

static int return_EIO(void)
{
return -EIO;
}

#define EIO_ERROR ((void *) (return_EIO))

static struct inode_operations bad_inode_ops =
{
.create = bad_inode_create
...etc...

The problem here is that the void cast causes return types to not be 
promoted, and for ops such as listxattr which expect more than 32 bits of
return value, the 32-bit -EIO is interpreted as a large positive 64-bit 
number, i.e. 0xfffa instead of 0xfffa.

This goes particularly badly when the return value is taken as a number of
bytes to copy into, say, a user's buffer for example...

I originally had coded up the fix by creating a return_EIO_ macro
for each return type, like this:

static int return_EIO_int(void)
{
return -EIO;
}
#define EIO_ERROR_INT ((void *) (return_EIO_int))

static struct inode_operations bad_inode_ops =
{
.create = EIO_ERROR_INT,
...etc...

but Al felt that it was probably better to create an EIO-returner for each 
actual op signature.  Since so few ops share a signature, I just went ahead 
& created an EIO function for each individual file & inode op that returns
a value.

Thanks,

-Eric

Signed-off-by: Eric Sandeen <[EMAIL PROTECTED]>

Index: linux-2.6.19/fs/bad_inode.c
===
--- linux-2.6.19.orig/fs/bad_inode.c
+++ linux-2.6.19/fs/bad_inode.c
@@ -14,59 +14,308 @@
 #include 
 #include 
 #include 
+#include 
 
-static int return_EIO(void)
+
+static loff_t bad_file_llseek(struct file *file, loff_t offset, int origin)
+{
+   return -EIO;
+}
+
+static ssize_t bad_file_read(struct file *filp, char __user *buf,
+   size_t size, loff_t *ppos)
+{
+return -EIO;
+}
+
+static ssize_t bad_file_write(struct file *filp, const char __user *buf,
+   size_t siz, loff_t *ppos)
+{
+return -EIO;
+}
+
+static ssize_t bad_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
+   unsigned long nr_segs, loff_t pos)
+{
+   return -EIO;
+}
+
+static ssize_t bad_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
+   unsigned long nr_segs, loff_t pos)
+{
+   return -EIO;
+}
+
+static int bad_file_readdir(struct file * filp, void * dirent,
+   filldir_t filldir)
+{
+   return -EIO;
+}
+
+static unsigned int bad_file_poll(struct file *filp, poll_table *wait)
+{
+   return -EIO;
+}
+
+static int bad_file_ioctl (struct inode * inode, struct file * filp,
+   unsigned int cmd, unsigned long arg)
 {
return -EIO;
 }
 
-#define EIO_ERROR ((void *) (return_EIO))
+static long bad_file_unlocked_ioctl(struct file *file, unsigned cmd,
+   unsigned long arg)
+{
+   return -EIO;
+}
+
+static long bad_file_compat_ioctl(struct file *file, unsigned int cmd,
+   unsigned long arg)
+{
+   return -EIO;
+}
+
+static int bad_file_mmap(struct file * file, struct vm_area_struct * vma)
+{
+   return -EIO;
+}
+
+static int bad_file_open(struct inode * inode, struct file * filp)
+{
+   return -EIO;
+}
+
+static int bad_file_flush(struct file *file, fl_owner_t id)
+{
+   return -EIO;
+}
+
+static int bad_file_release(struct inode * inode, struct file * filp)
+{
+   return -EIO;
+}
+
+static int bad_file_fsync(struct file * file, struct dentry *dentry,
+   int datasync)
+{
+   return -EIO;
+}
+
+static int bad_file_aio_fsync(struct kiocb *iocb, int datasync)
+{
+   return -EIO;
+}
+
+static int bad_file_fasync(int fd, struct file *filp, int on)
+{
+   return -EIO;
+}
+
+static int bad_file_lock(struct file *file, int cmd, struct file_lock *fl)
+{
+   return -EIO;
+}
+
+static ssize_t bad_file_sendfile(struct file *in_file, loff_t *ppos,
+   size_t count, read_actor_t actor, void *target)
+{
+   return -EIO;
+}
+
+static ssize_t bad_file_sendpage(struct file *file, struct page *page,
+   int off, size_t len, loff_t *pos, int more)
+{
+   return -EIO;
+}
+
+static unsigned long bad_file_get_unmapped_area(struct file *file,
+   unsigned long addr, unsigned long len,
+   unsigned long pgoff, unsigned long flags)
+{
+   return -EIO;
+}
+
+static int bad_file_check_flags(int flags)
+{
+   return -EIO;
+}
+
+static int bad_file_dir_notify(struct file * file, unsigned long arg)
+{
+   return -EIO;
+}
+
+static int bad_file_flock(struct file *filp, int cmd, struct file_lock *fl)
+{
+   return -EIO;
+}
+
+static ssize_t bad_file_splice_write(struct pipe_inode_info *pipe,
+   struct file *out, loff_t 

Re: [PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Stephen Rothwell wrote:
> Hi Eric,
> 
> On Wed, 03 Jan 2007 12:42:47 -0600 Eric Sandeen <[EMAIL PROTECTED]> wrote:
>> So here's the first stab at fixing it.  I'm sure there are style points
>> to be hashed out.  Putting all the functions as static inlines in a header
>> was just to avoid hundreds of lines of simple function declarations before
>> we get to the meat of bad_inode.c, but it's probably technically wrong to
>> put it in a header.  Also if putting a copyright on that trivial header file
>> is going overboard, just let me know.  Or if anyone has a less verbose
>> but still correct way to address this problem, I'm all ears.
> 
> Since the only uses of these functions is to take their addresses, the
> inline gains you nothing 

Hm, yes of course... my fingers just automatically type "static inline"
in header files I guess. :)

> and since the only uses are in the one file, you
> should just define them in that file.

Ok, will do.  That seems to be the consensus.

Thanks,

-Eric
-
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/

Re: [PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Stephen Rothwell]

Hi Eric,

On Wed, 03 Jan 2007 12:42:47 -0600 Eric Sandeen <[EMAIL PROTECTED]> wrote:
>
> So here's the first stab at fixing it.  I'm sure there are style points
> to be hashed out.  Putting all the functions as static inlines in a header
> was just to avoid hundreds of lines of simple function declarations before
> we get to the meat of bad_inode.c, but it's probably technically wrong to
> put it in a header.  Also if putting a copyright on that trivial header file
> is going overboard, just let me know.  Or if anyone has a less verbose
> but still correct way to address this problem, I'm all ears.

Since the only uses of these functions is to take their addresses, the
inline gains you nothing and since the only uses are in the one file, you
should just define them in that file.

--
Cheers,
Stephen Rothwell[EMAIL PROTECTED]
http://www.canb.auug.org.au/~sfr/


pgpWcClvqMma1.pgp
Description: PGP signature

[PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

CVE-2006-5753 is for a case where an inode can be marked bad, switching 
the ops to bad_inode_ops, which are all connected as:

static int return_EIO(void)
{
return -EIO;
}

#define EIO_ERROR ((void *) (return_EIO))

static struct inode_operations bad_inode_ops =
{
.create = bad_inode_create
...etc...

The problem here is that the void cast causes return types to not be 
promoted, and for ops such as listxattr which expect more than 32 bits of
return value, the 32-bit -EIO is interpreted as a large positive 64-bit 
number, i.e. 0xfffa instead of 0xfffa.

This goes particularly badly when the return value is taken as a number of
bytes to copy into, say, a user's buffer for example...

I originally had coded up the fix by creating a return_EIO_ macro
for each return type, like this:

static int return_EIO_int(void)
{
return -EIO;
}
#define EIO_ERROR_INT ((void *) (return_EIO_int))

static struct inode_operations bad_inode_ops =
{
.create = EIO_ERROR_INT,
...etc...

but Al felt that it was probably better to create an EIO-returner for each 
actual op signature.  Since so few ops share a signature, I just went ahead 
& created an EIO function for each individual file & inode op that returns
a value.

So here's the first stab at fixing it.  I'm sure there are style points
to be hashed out.  Putting all the functions as static inlines in a header
was just to avoid hundreds of lines of simple function declarations before 
we get to the meat of bad_inode.c, but it's probably technically wrong to 
put it in a header.  Also if putting a copyright on that trivial header file
is going overboard, just let me know.  Or if anyone has a less verbose
but still correct way to address this problem, I'm all ears.

Thanks,

-Eric

Signed-off-by: Eric Sandeen <[EMAIL PROTECTED]>

Index: linux-2.6.20-rc3/fs/bad_inode.h
===
--- /dev/null
+++ linux-2.6.20-rc3/fs/bad_inode.h
@@ -0,0 +1,266 @@
+/* fs/bad_inode.h
+ * bad_inode / bad_file internal definitions
+ *
+ * Copyright (C) 2007 Red Hat, Inc. All Rights Reserved.
+ * Written by Eric Sandeen ([EMAIL PROTECTED])
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+/* Bad file ops */
+
+static inline loff_t bad_file_llseek(struct file *file, loff_t offset,
+   int origin)
+{
+   return -EIO;
+}
+
+static inline ssize_t bad_file_read(struct file *filp, char __user *buf,
+   size_t size, loff_t *ppos)
+{
+return -EIO;
+}
+
+static inline ssize_t bad_file_write(struct file *filp, const char __user *buf,
+   size_t siz, loff_t *ppos)
+{
+return -EIO;
+}
+
+static inline ssize_t bad_file_aio_read(struct kiocb *iocb,
+   const struct iovec *iov, unsigned long nr_segs, loff_t pos)
+{
+   return -EIO;
+}
+
+static inline ssize_t bad_file_aio_write(struct kiocb *iocb,
+   const struct iovec *iov, unsigned long nr_segs, loff_t pos)
+{
+   return -EIO;
+}
+
+static inline int bad_file_readdir(struct file * filp, void * dirent,
+   filldir_t filldir)
+{
+   return -EIO;
+}
+
+static inline unsigned int bad_file_poll(struct file *filp, poll_table *wait)
+{
+   return -EIO;
+}
+
+static inline int bad_file_ioctl (struct inode * inode, struct file * filp,
+   unsigned int cmd, unsigned long arg)
+{
+   return -EIO;
+}
+
+static inline long bad_file_unlocked_ioctl(struct file *file, unsigned cmd,
+   unsigned long arg)
+{
+   return -EIO;
+}
+
+static inline long bad_file_compat_ioctl(struct file *file, unsigned int cmd,
+   unsigned long arg)
+{
+   return -EIO;
+}
+
+static inline int bad_file_mmap(struct file * file, struct vm_area_struct * 
vma)
+{
+   return -EIO;
+}
+
+static inline int bad_file_open(struct inode * inode, struct file * filp)
+{
+   return -EIO;
+}
+
+static inline int bad_file_flush(struct file *file, fl_owner_t id)
+{
+   return -EIO;
+}
+
+static inline int bad_file_release(struct inode * inode, struct file * filp)
+{
+   return -EIO;
+}
+
+static inline int bad_file_fsync(struct file * file, struct dentry *dentry,
+   int datasync)
+{
+   return -EIO;
+}
+
+static inline int bad_file_aio_fsync(struct kiocb *iocb, int datasync)
+{
+   return -EIO;
+}
+
+static inline int bad_file_fasync(int fd, struct file *filp, int on)
+{
+   return -EIO;
+}
+
+static inline int bad_file_lock(struct file *file, int cmd,
+   struct file_lock *fl)
+{
+   return -EIO;
+}
+
+static inline ssize_t bad_file_sendfile(struct file *in_file, loff_t *ppos,
+   size_t 

[PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

CVE-2006-5753 is for a case where an inode can be marked bad, switching 
the ops to bad_inode_ops, which are all connected as:

static int return_EIO(void)
{
return -EIO;
}

#define EIO_ERROR ((void *) (return_EIO))

static struct inode_operations bad_inode_ops =
{
.create = bad_inode_create
...etc...

The problem here is that the void cast causes return types to not be 
promoted, and for ops such as listxattr which expect more than 32 bits of
return value, the 32-bit -EIO is interpreted as a large positive 64-bit 
number, i.e. 0xfffa instead of 0xfffa.

This goes particularly badly when the return value is taken as a number of
bytes to copy into, say, a user's buffer for example...

I originally had coded up the fix by creating a return_EIO_TYPE macro
for each return type, like this:

static int return_EIO_int(void)
{
return -EIO;
}
#define EIO_ERROR_INT ((void *) (return_EIO_int))

static struct inode_operations bad_inode_ops =
{
.create = EIO_ERROR_INT,
...etc...

but Al felt that it was probably better to create an EIO-returner for each 
actual op signature.  Since so few ops share a signature, I just went ahead 
 created an EIO function for each individual file  inode op that returns
a value.

So here's the first stab at fixing it.  I'm sure there are style points
to be hashed out.  Putting all the functions as static inlines in a header
was just to avoid hundreds of lines of simple function declarations before 
we get to the meat of bad_inode.c, but it's probably technically wrong to 
put it in a header.  Also if putting a copyright on that trivial header file
is going overboard, just let me know.  Or if anyone has a less verbose
but still correct way to address this problem, I'm all ears.

Thanks,

-Eric

Signed-off-by: Eric Sandeen [EMAIL PROTECTED]

Index: linux-2.6.20-rc3/fs/bad_inode.h
===
--- /dev/null
+++ linux-2.6.20-rc3/fs/bad_inode.h
@@ -0,0 +1,266 @@
+/* fs/bad_inode.h
+ * bad_inode / bad_file internal definitions
+ *
+ * Copyright (C) 2007 Red Hat, Inc. All Rights Reserved.
+ * Written by Eric Sandeen ([EMAIL PROTECTED])
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+/* Bad file ops */
+
+static inline loff_t bad_file_llseek(struct file *file, loff_t offset,
+   int origin)
+{
+   return -EIO;
+}
+
+static inline ssize_t bad_file_read(struct file *filp, char __user *buf,
+   size_t size, loff_t *ppos)
+{
+return -EIO;
+}
+
+static inline ssize_t bad_file_write(struct file *filp, const char __user *buf,
+   size_t siz, loff_t *ppos)
+{
+return -EIO;
+}
+
+static inline ssize_t bad_file_aio_read(struct kiocb *iocb,
+   const struct iovec *iov, unsigned long nr_segs, loff_t pos)
+{
+   return -EIO;
+}
+
+static inline ssize_t bad_file_aio_write(struct kiocb *iocb,
+   const struct iovec *iov, unsigned long nr_segs, loff_t pos)
+{
+   return -EIO;
+}
+
+static inline int bad_file_readdir(struct file * filp, void * dirent,
+   filldir_t filldir)
+{
+   return -EIO;
+}
+
+static inline unsigned int bad_file_poll(struct file *filp, poll_table *wait)
+{
+   return -EIO;
+}
+
+static inline int bad_file_ioctl (struct inode * inode, struct file * filp,
+   unsigned int cmd, unsigned long arg)
+{
+   return -EIO;
+}
+
+static inline long bad_file_unlocked_ioctl(struct file *file, unsigned cmd,
+   unsigned long arg)
+{
+   return -EIO;
+}
+
+static inline long bad_file_compat_ioctl(struct file *file, unsigned int cmd,
+   unsigned long arg)
+{
+   return -EIO;
+}
+
+static inline int bad_file_mmap(struct file * file, struct vm_area_struct * 
vma)
+{
+   return -EIO;
+}
+
+static inline int bad_file_open(struct inode * inode, struct file * filp)
+{
+   return -EIO;
+}
+
+static inline int bad_file_flush(struct file *file, fl_owner_t id)
+{
+   return -EIO;
+}
+
+static inline int bad_file_release(struct inode * inode, struct file * filp)
+{
+   return -EIO;
+}
+
+static inline int bad_file_fsync(struct file * file, struct dentry *dentry,
+   int datasync)
+{
+   return -EIO;
+}
+
+static inline int bad_file_aio_fsync(struct kiocb *iocb, int datasync)
+{
+   return -EIO;
+}
+
+static inline int bad_file_fasync(int fd, struct file *filp, int on)
+{
+   return -EIO;
+}
+
+static inline int bad_file_lock(struct file *file, int cmd,
+   struct file_lock *fl)
+{
+   return -EIO;
+}
+
+static inline ssize_t bad_file_sendfile(struct file *in_file, loff_t *ppos,
+   size_t 

[UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Take 2... all in one file.  I suppose I really did know better than 
to create that new header.   ;-) 

Better?

---

CVE-2006-5753 is for a case where an inode can be marked bad, switching 
the ops to bad_inode_ops, which are all connected as:

static int return_EIO(void)
{
return -EIO;
}

#define EIO_ERROR ((void *) (return_EIO))

static struct inode_operations bad_inode_ops =
{
.create = bad_inode_create
...etc...

The problem here is that the void cast causes return types to not be 
promoted, and for ops such as listxattr which expect more than 32 bits of
return value, the 32-bit -EIO is interpreted as a large positive 64-bit 
number, i.e. 0xfffa instead of 0xfffa.

This goes particularly badly when the return value is taken as a number of
bytes to copy into, say, a user's buffer for example...

I originally had coded up the fix by creating a return_EIO_TYPE macro
for each return type, like this:

static int return_EIO_int(void)
{
return -EIO;
}
#define EIO_ERROR_INT ((void *) (return_EIO_int))

static struct inode_operations bad_inode_ops =
{
.create = EIO_ERROR_INT,
...etc...

but Al felt that it was probably better to create an EIO-returner for each 
actual op signature.  Since so few ops share a signature, I just went ahead 
 created an EIO function for each individual file  inode op that returns
a value.

Thanks,

-Eric

Signed-off-by: Eric Sandeen [EMAIL PROTECTED]

Index: linux-2.6.19/fs/bad_inode.c
===
--- linux-2.6.19.orig/fs/bad_inode.c
+++ linux-2.6.19/fs/bad_inode.c
@@ -14,59 +14,308 @@
 #include linux/time.h
 #include linux/smp_lock.h
 #include linux/namei.h
+#include linux/poll.h
 
-static int return_EIO(void)
+
+static loff_t bad_file_llseek(struct file *file, loff_t offset, int origin)
+{
+   return -EIO;
+}
+
+static ssize_t bad_file_read(struct file *filp, char __user *buf,
+   size_t size, loff_t *ppos)
+{
+return -EIO;
+}
+
+static ssize_t bad_file_write(struct file *filp, const char __user *buf,
+   size_t siz, loff_t *ppos)
+{
+return -EIO;
+}
+
+static ssize_t bad_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
+   unsigned long nr_segs, loff_t pos)
+{
+   return -EIO;
+}
+
+static ssize_t bad_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
+   unsigned long nr_segs, loff_t pos)
+{
+   return -EIO;
+}
+
+static int bad_file_readdir(struct file * filp, void * dirent,
+   filldir_t filldir)
+{
+   return -EIO;
+}
+
+static unsigned int bad_file_poll(struct file *filp, poll_table *wait)
+{
+   return -EIO;
+}
+
+static int bad_file_ioctl (struct inode * inode, struct file * filp,
+   unsigned int cmd, unsigned long arg)
 {
return -EIO;
 }
 
-#define EIO_ERROR ((void *) (return_EIO))
+static long bad_file_unlocked_ioctl(struct file *file, unsigned cmd,
+   unsigned long arg)
+{
+   return -EIO;
+}
+
+static long bad_file_compat_ioctl(struct file *file, unsigned int cmd,
+   unsigned long arg)
+{
+   return -EIO;
+}
+
+static int bad_file_mmap(struct file * file, struct vm_area_struct * vma)
+{
+   return -EIO;
+}
+
+static int bad_file_open(struct inode * inode, struct file * filp)
+{
+   return -EIO;
+}
+
+static int bad_file_flush(struct file *file, fl_owner_t id)
+{
+   return -EIO;
+}
+
+static int bad_file_release(struct inode * inode, struct file * filp)
+{
+   return -EIO;
+}
+
+static int bad_file_fsync(struct file * file, struct dentry *dentry,
+   int datasync)
+{
+   return -EIO;
+}
+
+static int bad_file_aio_fsync(struct kiocb *iocb, int datasync)
+{
+   return -EIO;
+}
+
+static int bad_file_fasync(int fd, struct file *filp, int on)
+{
+   return -EIO;
+}
+
+static int bad_file_lock(struct file *file, int cmd, struct file_lock *fl)
+{
+   return -EIO;
+}
+
+static ssize_t bad_file_sendfile(struct file *in_file, loff_t *ppos,
+   size_t count, read_actor_t actor, void *target)
+{
+   return -EIO;
+}
+
+static ssize_t bad_file_sendpage(struct file *file, struct page *page,
+   int off, size_t len, loff_t *pos, int more)
+{
+   return -EIO;
+}
+
+static unsigned long bad_file_get_unmapped_area(struct file *file,
+   unsigned long addr, unsigned long len,
+   unsigned long pgoff, unsigned long flags)
+{
+   return -EIO;
+}
+
+static int bad_file_check_flags(int flags)
+{
+   return -EIO;
+}
+
+static int bad_file_dir_notify(struct file * file, unsigned long arg)
+{
+   return -EIO;
+}
+
+static int bad_file_flock(struct file *filp, int cmd, struct file_lock *fl)
+{
+   return -EIO;
+}
+
+static ssize_t bad_file_splice_write(struct pipe_inode_info 

Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Andrew Morton]

On Wed, 03 Jan 2007 17:46:00 -0600
Eric Sandeen [EMAIL PROTECTED] wrote:

 Take 2... all in one file.  I suppose I really did know better than 
 to create that new header.   ;-) 
 
 Better?
 
 ---
 
 CVE-2006-5753 is for a case where an inode can be marked bad, switching 
 the ops to bad_inode_ops, which are all connected as:
 
 static int return_EIO(void)
 {
 return -EIO;
 }
 
 #define EIO_ERROR ((void *) (return_EIO))
 
 static struct inode_operations bad_inode_ops =
 {
 .create = bad_inode_create
 ...etc...
 
 The problem here is that the void cast causes return types to not be 
 promoted, and for ops such as listxattr which expect more than 32 bits of
 return value, the 32-bit -EIO is interpreted as a large positive 64-bit 
 number, i.e. 0xfffa instead of 0xfffa.
 
 This goes particularly badly when the return value is taken as a number of
 bytes to copy into, say, a user's buffer for example...
 
 I originally had coded up the fix by creating a return_EIO_TYPE macro
 for each return type, like this:
 
 static int return_EIO_int(void)
 {
   return -EIO;
 }
 #define EIO_ERROR_INT ((void *) (return_EIO_int))
 
 static struct inode_operations bad_inode_ops =
 {
   .create = EIO_ERROR_INT,
 ...etc...
 
 but Al felt that it was probably better to create an EIO-returner for each 
 actual op signature.  Since so few ops share a signature, I just went ahead 
  created an EIO function for each individual file  inode op that returns
 a value.
 

Al is correct, of course.  But the patch takes bad_inode.o from 280 up to 703
bytes, which is a bit sad for some cosmetic thing which nobody ever looks
at or modifies.

Perhaps you can do

static int return_EIO_int(void)
{
return -EIO;
}

static int bad_file_release(struct inode * inode, struct file * filp)
__attribute__((alias(return_EIO_int)));
static int bad_file_fsync(struct inode * inode, struct file * filp)
__attribute__((alias(return_EIO_int)));

etcetera?
-
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/

Re: [PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Stephen Rothwell]

Hi Eric,

On Wed, 03 Jan 2007 12:42:47 -0600 Eric Sandeen [EMAIL PROTECTED] wrote:

 So here's the first stab at fixing it.  I'm sure there are style points
 to be hashed out.  Putting all the functions as static inlines in a header
 was just to avoid hundreds of lines of simple function declarations before
 we get to the meat of bad_inode.c, but it's probably technically wrong to
 put it in a header.  Also if putting a copyright on that trivial header file
 is going overboard, just let me know.  Or if anyone has a less verbose
 but still correct way to address this problem, I'm all ears.

Since the only uses of these functions is to take their addresses, the
inline gains you nothing and since the only uses are in the one file, you
should just define them in that file.

--
Cheers,
Stephen Rothwell[EMAIL PROTECTED]
http://www.canb.auug.org.au/~sfr/


pgpWcClvqMma1.pgp
Description: PGP signature

Re: [PATCH] fix memory corruption from misinterpreted bad_inode_ops return values

[Eric Sandeen]

Stephen Rothwell wrote:
 Hi Eric,
 
 On Wed, 03 Jan 2007 12:42:47 -0600 Eric Sandeen [EMAIL PROTECTED] wrote:
 So here's the first stab at fixing it.  I'm sure there are style points
 to be hashed out.  Putting all the functions as static inlines in a header
 was just to avoid hundreds of lines of simple function declarations before
 we get to the meat of bad_inode.c, but it's probably technically wrong to
 put it in a header.  Also if putting a copyright on that trivial header file
 is going overboard, just let me know.  Or if anyone has a less verbose
 but still correct way to address this problem, I'm all ears.
 
 Since the only uses of these functions is to take their addresses, the
 inline gains you nothing 

Hm, yes of course... my fingers just automatically type static inline
in header files I guess. :)

 and since the only uses are in the one file, you
 should just define them in that file.

Ok, will do.  That seems to be the consensus.

Thanks,

-Eric
-
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/