Re: [uml-devel] [RFC] PATCH 3/4 - Time virtualization : PTRACE_SYSCALL_MASK

[Blaisorblade <blaisorblade@yahoo.it>]

On Tuesday 25 April 2006 18:29, Jeff Dike wrote:
> On Fri, Apr 21, 2006 at 08:34:52PM +0200, Blaisorblade wrote:
> > >  #define PTRACE_GET_THREAD_AREA    25
> > >  #define PTRACE_SET_THREAD_AREA    26
> > > +#define PTRACE_SYSCALL_MASK	  27
> >
> > I think there could be a reason we skipped that for SYSEMU - that's to
> > see. Also, if this capability will be implemented in other archs, we
> > should use the 0x4200-0x4300 range for it.
>
> Yeah, we need to decide somewhat carefully which number to use.
>
> > > +		for(i = NR_syscalls; i < len * 8; i++){
> > > +			get_user(c, &mask[i / 8]);
> >
> > This get_user() inside a loop is poor, it could slow down a valid call.
> > It'd be simpler to copy the mask from userspace in a local variable (with
> > 400 syscalls that's 50 bytes, i.e. fully ok), and then perform the
> > checks, if wanted (I disagree with Heiko's message, this check is needed
> > sometimes - see  my response to that).
>
> Agree, except that we need to be careful about when userspace knows
> about more system calls than the kernel.  We should copy-user as many
> bits as the kernel knows about (or the process passes in, which ever
> is less) and if the process knows about more system calls than the
> kernel, the extra bits should be checked (maybe in a get_user(c, ...)
> loop) to make sure that special treatment isn't being requested for
> unknown syscalls.

Yes, that's exactly what I thought. The get_user() loop isn't that nice but 
that's possibly a minor point.

> > And only after that set all at once child->syscall_mask. You copy twice
> > that little quantity of data but that's not at all time-critical, and
> > you're forced to do that to avoid partial updates; btw you've saved
> > getting twice the content from userspace (slow when address spaces are
> > distinct, like for 4G/4G or SKAS implementation of copy_from_user).

> Yup.

> > Actually we would copy the whole struct in my API proposal (as I've
> > described in the other message, we need to pass another param IMHO,
> > so we'd pack them in a struct and pass its address).

> You mean adding a fifth argument to ptrace?  I don't really like that
> idea.  We could either make two new PTRACE_* operations (I don't like
> the MASK_STRICT_VERIFY option since that seems unnecessary and
> fragile) or make the data argument something like this

> Except that passing pointers to pointers into system calls seems like
> a bad idea - it makes ptrace look (more) like ioctl.  So, you'd want
> something like
> 	struct {
> 		int flag;
> 		char mask[(NR_syscalls + 7)/8];
> 	}
>
> then you'd want the length back in data so you know how much data the
> process is giving you.

Yes, this is what I mean.

> But then, you'll read the smaller of the 
> kernel's and process's version of the structure, and if the process
> one is bigger, you need to read the extra bits to sanity-check them.
> Given that you'll need this extra treatment,

You need this treatment anyway - above we're passing a pointer to a bitstring, 
here we're passing a pointer to a struct containing a bitstring, in both 
cases we must copy in the right amount of bytes.

> I think it's simpler to 
> just leave the addr argument as a pointer to the bits and add an extra
> ptrace op.

If we can do without MASK_STRICT_VERIFY, that works fully, and anyway it's 
simpler - however, say, when running strace -e read,tee (sys_tee will soon be 
added, it seems) this call would fail, while it would be desirable to have it 
work as strace -e read.

MASK_STRICT_VERIFY isn't necessarily the best solution, but if userspace must 
search the maximum allowed syscall by multiple attempts, we've still a bad 
API.

Probably, a better option (_instead_ of MASK_STRICT_VERIFY) would be to return 
somewhere an "extended error code" saying which is the last allowed syscall 
or (better) which is the first syscall which failed. I.e. if there is strace 
-e read,splice,tee and nor splice nor tee are supported, then this value 
would be __NR_splice and strace (or any app) could then decide what to do.

To do that we need again a structure with a field where to store the code 
(which _must_ be at the beginning).

But this is cleaner than saying to the kernel "interpret what I say if I'm 
wrong", and I said above the complexity is the same when copying the 
structure.

And I'd use this together with the "two ptrace codes" idea.
Let's say we'll use PTRACE_TRACE_ONLY or PTRACE_TRACE_EXCEPT.

Another possibility (which however implies implementation for all 
architectures) is to put these two requests between ptrace options (i.e. 
PTRACE_SETOPTIONS), where it logically belongs (and this is the only point 
reason to do it this way); however we have then only one parameter, which 
would become then a pointer to such a structure:

struct {
	int ret_code;
	int mask_len;
	char mask[];
};
-- 
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade

	

	
		
___________________________________ 
Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB 
http://mail.yahoo.it
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/