lkml.org 
[lkml]   [2006]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [uml-devel] [RFC] PATCH 3/4 - Time virtualization : PTRACE_SYSCALL_MASK
Date
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/

\
 
 \ /
  Last update: 2006-04-26 17:50    [W:0.079 / U:0.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site