Re: [PATCH] Fix kunmap() argument in sg_miter_stop

Ingo Molnar
2008-11-18T08:27:51Z

* Jens Axboe <jens.axboe@oracle.com> wrote:

> On Mon, Nov 17 2008, Linus Torvalds wrote:
> >
> >
> > On Mon, 17 Nov 2008, Jens Axboe wrote:
> > >
> > > Any opinions on the kunmap/kunmap_atomic pointer checking? It's a bit
> > > ugly that we have to enforce a void * rule for kunmap_atomic(),
> >
> > I don't think that's a "bit ugly". I think it's unacceptable.
> >
> > Making sure we pass in "struct page" to kunmap() sounds good, but the
> > kunmap_atomic() part just sounds insane.
>
> It's been the primary source of bugs that I have seen. The xen and
> sg iter bug were kunmap() variants though, but otherwise I've mostly
> seen the opposite. But it is ugly, no doubt about it. I can't think
> of a better way to attempt to warn about it though, so if you really
> dislike it I'll just drop the _atomic() bits.

The main ugliness comes from the tons of void * type casts that the
kunmap_atomic() type check forces. Type casts are just as dangerous
(and ugly) as type mismatches. (more dangerous in fact)

Perhaps we could try an opt-in 'type filter' approach instead. See
kernel/tracing/trace.h's trace_assign_type()'s type checking magic for
an example of how to do it.

( but it's a bit tricky here because we want to filter void * from
struct page * - i'm not sure gcc will recognize them as incompatible
types. )

Ingo