linux-kernel - Re: types for storing instruction pointers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20090608141209.GC14234@elte.hu>
Date:	Mon, 8 Jun 2009 16:12:09 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Christoph Hellwig <hch@....de>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: types for storing instruction pointers


* Christoph Hellwig <hch@....de> wrote:

> On Sun, Jun 07, 2009 at 03:25:16PM +0200, Ingo Molnar wrote:
> > 
> > * Christoph Hellwig <hch@....de> wrote:
> > 
> > > Currently the _THIS_IP_ and _RET_IP_ macros aded for lockdep but 
> > > now available from kernel.org case the instruction pointer to an 
> > > unsigned long.  But the %pf/%pF format for printing them want a 
> > > pointer of some sort.  That's a pretty nasty situation for tracers 
> > > - can we standardize on one type for it?
> > 
> > Yes but what's the practical problem exactly? Could you cite an 
> > example?
> 
> The simplest tracer in xfs showing this is the following:
> 
> /*
>  * ilock/iolock tracer
>  *
>  * Reports the inode, operation, flags and caller for each operation
>  * on the inode locks.
>  */
> TRACE_EVENT(xfs_ilock,
> 	TP_PROTO(struct xfs_inode *ip, const char *op, unsigned lockflags,
> 		 unsigned long caller_ip),
> 	TP_ARGS(ip, op, lockflags, caller_ip),
> 
> 	TP_STRUCT__entry(
> 		__field(dev_t, dev)
> 		__field(xfs_ino_t, ino)
> 		__field(const char *, op)
> 		__field(int, lockflags)
> 		__field(unsigned long, caller_ip)
> 	),
> 
> 	TP_fast_assign(
> 		__entry->dev = VFS_I(ip)->i_sb->s_dev;
> 		__entry->ino = ip->i_ino;
> 		__entry->op = op;
> 		__entry->lockflags = lockflags;
> 		__entry->caller_ip = caller_ip;
> 	),
> 
> 	TP_printk("dev %d:%d ino 0x%lld %s %s by %pF",
> 		  MAJOR(__entry->dev), MINOR(__entry->dev),
> 		  __entry->ino,
> 		  __entry->op,
> 		  __print_flags(__entry->lockflags, "|", XFS_LOCK_FLAGS),
> 		  (void *)__entry->caller_ip)
> );
> 
> It has the following callers:
> 
> 	trace_xfs_ilock(ip, "lock", lock_flags, _RET_IP_);
> 	trace_xfs_ilock(ip, "lock_nowait", lock_flags, _RET_IP_);
> 	trace_xfs_ilock(ip, "unlock", lock_flags, _RET_IP_);
> 	trace_xfs_ilock(ip, "demote", lock_flags, _RET_IP_);
> 
> Basically everything obtained via _RET_IP_/_THIS_IP_ needs to be 
> casted.  Given that both need to case the their return value to a 
> pointer that's rather unfortunately.  Life would be much easier if 
> _RET_IP_/_THIS_IP_ just returned a pointer (probably just a void 
> pointer, maybe with a fancy typedef to make it clear we're dealing 
> with an instruction pointer here).

Yeah, there's really no coherency in this area anywhere in the 
kernel. IPs are often represented as unsigned long, in symbol lookup 
and elsewhere - that is where lockdep got that principle from. There 
it hurts if we do this change - i.e. we'd just shift the point of 
'friction' between types from tracing to the symbol lookup code and 
to platform code.

So i'd fully agree with making all that a (void *), but then you 
need to hunt down all the other uses of code addresses as well and 
standardize the thing all across the kernel. Not a small patch :-)

( Plus turning it into a separate type probably makes sense as well, 
  as there are platforms where function pointers are different from 
  regular pointers so keeping them sorted separate is good. )

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/