Kill a sparse warning in 2.6.9-rc3.
Signed-off-by: Arun Sharma <[email protected]>
===== fs/binfmt_elf.c 1.88 vs edited =====
--- 1.88/fs/binfmt_elf.c Wed Sep 22 13:34:05 2004
+++ edited/fs/binfmt_elf.c Wed Oct 6 13:16:37 2004
@@ -1032,7 +1032,7 @@
*/
static int dump_write(struct file *file, const void *addr, int nr)
{
- return file->f_op->write(file, addr, nr, &file->f_pos) == nr;
+ return file->f_op->write(file, (const char __user *) addr, nr, &file->f_pos) == nr;
}
static int dump_seek(struct file *file, off_t off)
On Wed, 06 Oct 2004 15:45:02 PDT, Arun Sharma said:
> The attached patch kills a sparse warning in binfmt_elf.c:dump_write() by
> adding a __user annotation.
> static int dump_write(struct file *file, const void *addr, int nr)
> {
> - return file->f_op->write(file, addr, nr, &file->f_pos) == nr;
> + return file->f_op->write(file, (const char __user *) addr, nr, &file->f_pos) == nr;
> }
wouldn't it be more useful to put the annotation into the *prototype* for
the dump_write() function, so that we get sparse typechecking for the
caller(s) of this function? Your fix just kills the warning - when the *real*
problem is that we're called with a 'void *' that we then cast to something
without any real double check on what it is....
On 10/7/2004 11:11 AM, [email protected] wrote:
> On Wed, 06 Oct 2004 15:45:02 PDT, Arun Sharma said:
>
>> The attached patch kills a sparse warning in binfmt_elf.c:dump_write() by
>> adding a __user annotation.
>
>> static int dump_write(struct file *file, const void *addr, int nr)
>> {
>> - return file->f_op->write(file, addr, nr, &file->f_pos) == nr;
>> + return file->f_op->write(file, (const char __user *) addr, nr, &file->f_pos) == nr;
>> }
>
> wouldn't it be more useful to put the annotation into the *prototype* for
> the dump_write() function, so that we get sparse typechecking for the
> caller(s) of this function? Your fix just kills the warning - when the *real*
> problem is that we're called with a 'void *' that we then cast to something
> without any real double check on what it is....
dump_write() is a static function without a prototype.
-Arun
On Thu, 07 Oct 2004 11:49:24 PDT, Arun Sharma said:
> >> static int dump_write(struct file *file, const void *addr, int nr)
> >> {
> >> - return file->f_op->write(file, addr, nr, &file->f_pos) == nr;
> >> + return file->f_op->write(file, (const char __user *) addr, nr, &file->f
_pos) == nr;
> >> }
> >
> > wouldn't it be more useful to put the annotation into the *prototype* for
> > the dump_write() function, so that we get sparse typechecking for the
> > caller(s) of this function? Your fix just kills the warning - when the *re
al*
> > problem is that we're called with a 'void *' that we then cast to something
> > without any real double check on what it is....
>
> dump_write() is a static function without a prototype.
static int dump_write(struct file *file, const char __user *addr, int nr)
{
return file->f_op->write(file, addr, nr, &file->f_pos) == nr;
}
And then go find the callers and make sure what they're passing is a
'const char __user *' rather than a 'const void *'....
On Thu, 07 Oct 2004 12:01:19 PDT, Arun Sharma said:
> Many callers in binfmt_elf.c are passing pointers that are kernel addresses.
> But file_operations->write() is expecting a __user pointer. The intention of
> the cast is to explicitly say that's an okay thing to do.
Right. My point was that if you're fixing the warning, you should do the
*whole* job by making sure that what *you* got in that 'void *' is a __user
pointer, rather than just saying "we know write() wants a __user".
So for instance, up a level in writenote():
#define DUMP_WRITE(addr, nr) do { if (!dump_write(file, (addr), (nr))) return 0; } while(0)
#define DUMP_SEEK(off) do { if (!dump_seek(file, (off))) return 0; } while(0)
static int writenote(struct memelfnote *men, struct file *file)
{
struct elf_note en;
en.n_namesz = strlen(men->name) + 1;
en.n_descsz = men->datasz;
en.n_type = men->type;
DUMP_WRITE(&en, sizeof(en));
DUMP_WRITE(men->name, en.n_namesz);
Is men->name a __user pointer? Should it be? Can it be something else?
struct memelfnote
{
const char *name;
int type;
unsigned int datasz;
void *data;
};
Hmm.. there it's a 'const char *name', but we feed it to a 'const void *'
and then you cast it to a __user. Either that *name should be __user
as well, or you tagged something as a __user that might not be.
*That*s what I'm talking about....
On 10/7/2004 11:54 AM, [email protected] wrote:
> And then go find the callers and make sure what they're passing is a
> 'const char __user *' rather than a 'const void *'....
Many callers in binfmt_elf.c are passing pointers that are kernel addresses. But file_operations->write() is expecting a __user pointer. The intention of the cast is to explicitly say that's an okay thing to do.
-Arun
On Thu, 07 Oct 2004 14:54:57 -0400
[email protected] wrote:
> > dump_write() is a static function without a prototype.
>
> static int dump_write(struct file *file, const char __user *addr, int nr)
> {
> return file->f_op->write(file, addr, nr, &file->f_pos) == nr;
> }
>
> And then go find the callers and make sure what they're passing is a
> 'const char __user *' rather than a 'const void *'....
The caller's aren't, that is the point. They run dump_write()
with set_fs(KERNEL_DS), which allows kernel pointers to be treated
as user ones in system call handling paths, which is why the cast
is needed somewhere.
On Thu, 07 Oct 2004 12:16:23 PDT, "David S. Miller" said:
> The caller's aren't, that is the point. They run dump_write()
> with set_fs(KERNEL_DS), which allows kernel pointers to be treated
> as user ones in system call handling paths, which is why the cast
> is needed somewhere.
Right - and my point is that putting one cast way down at the bottom
to quiet a warning doesn't do much good - the cast should be pushed
up to as close to that set_fs() as feasible. Otherwise if some other,
new, caller surfaces and bogusly passes something *else* in that
void * pointer, it gets a lot harder for sparse and similar to do their
jobs.
On 10/7/2004 12:16 PM, [email protected] wrote:
> Hmm.. there it's a 'const char *name', but we feed it to a 'const void *'
> and then you cast it to a __user.
I'm not very concerned about const char * being cast to const void *. That's not the focus of the patch[1]. The focus of the patch is really __user.
> Either that *name should be __user
> as well, or you tagged something as a __user that might not be.
>
Certain interfaces such as get_user() will fail if you pass a kernel pointer and unconditionally cast it to __user (without doing a set_fs(KERNEL_DS)). But file_operations->write() is not one of them and hence the patch.
-Arun
[1] If you insist on this, you should change all the calls to memcpy() as well.