Re: [Intel-gfx] kernel oops loading i915 after "x86/asm: Pin sensitive CR4 bits" (873d50d58)
Hi, On Wed, Jul 10, 2019 at 01:44:17PM +0800, Xi Ruoyao wrote: > Hello, > > When I try to build and run the latest mainline kernel, it Oops loading i915 > module: > > BUG: unable to handle page fault for address: 9edc1598 > #PF: supervisor write access in kernel mode > #PF: error_code(0x0003) - permissions violation > PGD 1a20c067 P4D 1a20c067 PUD 1a20d063 PMD 800019e000e1 > Oops: 0003 [#1] SMP PTI > > The complete log is attached. > > Bisection tells "x86/asm: Pin sensitive CR4 bits" (873d50d58) is the first > "bad" > commit. I can revert it and also "x86/asm: Pin sensitive CR0 bits" > (8dbec27a2) > to make the kernel "seems to" work. > > I'm not a kernel expert so I can't tell if there is a bug in Kees' patch, or > his > patch exploits a bug in i915 or module loader. This seems to be a kernel bug introduced after v5.2, see https://lore.kernel.org/lkml/CAHk-=wjh+h_-fd-gJz=wor42znmqq46qnb90jyfzqmklslf...@mail.gmail.com/ -- ldv ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 2/5] drm: Add private data field to trace control block
On Mon, Sep 07, 2015 at 08:23:57PM +0200, Patrik Jakobsson wrote:
> On Mon, Sep 7, 2015 at 6:51 PM, Dmitry V. Levin wrote:
> > On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
> > [...]
> >> Here's my take on it (I assume it needs some discussion):
> >>
> >> int
> >> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
> >> {
> >> /* A free callback is required before setting private data and
> >> private
> >>* data must be set back to NULL before being set again.
> >>*/
> >
> > I think a single function initializing both _priv_data and _free_priv_data
> > would suffice:
> >
> > int
> > set_tcb_priv_data(struct tcb *tcp, void *priv_data,
> > void (*free_priv_data)(void *))
> > {
> > if (tcp->_priv_data)
> > return -1;
> >
> > tcp->_free_priv_data = free_priv_data;
> > tcp->_priv_data = priv_data;
> >
> > return 0;
> > }
>
> Sure, and since they always come in a pairs it might be even better. If it
> turns
> out we need it split up it is easily done later.
JFYI, I've finalized and merged this set_tcb_priv_data interface.
--
ldv
pgp4iyIZxJS8j.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 2/5] drm: Add private data field to trace control block
On Mon, Sep 07, 2015 at 08:23:57PM +0200, Patrik Jakobsson wrote:
> On Mon, Sep 7, 2015 at 6:51 PM, Dmitry V. Levin wrote:
> > On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
> > [...]
> >> Here's my take on it (I assume it needs some discussion):
> >>
> >> int
> >> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
> >> {
> >> /* A free callback is required before setting private data and
> >> private
> >>* data must be set back to NULL before being set again.
> >>*/
> >
> > I think a single function initializing both _priv_data and _free_priv_data
> > would suffice:
> >
> > int
> > set_tcb_priv_data(struct tcb *tcp, void *priv_data,
> > void (*free_priv_data)(void *))
> > {
> > if (tcp->_priv_data)
> > return -1;
> >
> > tcp->_free_priv_data = free_priv_data;
> > tcp->_priv_data = priv_data;
> >
> > return 0;
> > }
>
> Sure, and since they always come in a pairs it might be even better. If it
> turns
> out we need it split up it is easily done later.
The discussion seems to be stalled.
Patrik, would you like to prepare a patch?
--
ldv
pgpJLCZWgilyc.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 3/5] drm: Add dispatcher and driver identification for DRM
On Fri, Sep 11, 2015 at 12:57:06PM +0200, Patrik Jakobsson wrote:
> On Tue, Sep 08, 2015 at 03:36:25AM +0300, Dmitry V. Levin wrote:
> > On Mon, Aug 24, 2015 at 02:42:48PM +0200, Patrik Jakobsson wrote:
[...]
> > > +static char *drm_get_driver_name(struct tcb *tcp)
> > > +{
> > > + char path[PATH_MAX];
> > > + char link[PATH_MAX];
> > > + int ret;
> > > +
> > > + if (getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1) < 0)
> > > + return NULL;
> > > +
> > > + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
> > > + basename(path));
> >
> > if (snprintf(link, sizeof(link), ...) >= sizeof(link))
> > return NULL;
> >
>
> According to manpage snprintf never returns more than the specified size - 1.
Really?
"The functions snprintf() and vsnprintf() do not write more than size
bytes (including the terminating null byte ('\0')). If the output was
truncated due to this limit, then the return value is the number of
characters (excluding the terminating null byte) which would have been
written to the final string if enough space had been available. Thus,
a return value of size or more means that the output was truncated."
--
ldv
pgpGbjcYBUQdC.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 4/5] drm: Add decoding of i915 ioctls
On Fri, Sep 11, 2015 at 01:31:02PM +0200, Patrik Jakobsson wrote:
> On Tue, Sep 08, 2015 at 04:18:11AM +0300, Dmitry V. Levin wrote:
> > On Mon, Aug 24, 2015 at 02:42:49PM +0200, Patrik Jakobsson wrote:
> > > +static int i915_getparam(struct tcb *tcp, const unsigned int code, long
> > > arg)
> > > +{
> > > + struct drm_i915_getparam param;
> > > + int value;
> > > +
> > > + if (umove(tcp, arg, ))
> > > + return RVAL_DECODED;
> > > +
> > > + if (entering(tcp)) {
> > > + tprints(", {param=");
> > > + printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
> > > + } else if (exiting(tcp)) {
> > > + if (umove(tcp, (long)param.value, ))
> > > + return RVAL_DECODED;
> >
> > Since part of param has already been printed, RVAL_DECODED shouldn't be
> > returned here. For the same reason, RVAL_DECODED shouldn't be returned
> > earlier in this function.
>
> The usage of RVAL_DECODED is quite confusing. RVAL_DECODED to me should be "We
> decoded this don't do any fallback". How did you intent it to be used?
RVAL_DECODED itself is trivial: by setting this flag parser tells its
caller that decoding is finished on entering and it shouldn't be called on
exiting of this syscall. Setting this flag on exiting has no effect.
With regards to ioctl parsers, there might be some confusion because they
also have this unusual return code +1 semantics. In this example, the
problem with "return RVAL_DECODED" statements is that if they happen on
exiting, it means that something has already been printed on entering
already and by returning RVAL_DECODED parser tells its caller to perform
the default action that also prints something.
That is, ioctl parser should return
- on entering:
+ any value (it's ignored) without RVAL_DECODED flag set:
continue processing on exiting;
+ RVAL_DECODED:
skip processing on exiting and tell the caller
to perform default processing on entering;
+ RVAL_DECODED | (1 + RVAL_*):
skip processing on exiting and tell the caller
to skip default processing;
- on exiting:
+ 0 (with or without RVAL_DECODED set, it's ignored anyway):
tell the caller to perform default processing on exiting;
+ 1 + RVAL_* (RVAL_DECODED is ignored):
tell the caller to skip default processing.
> > > + break;
> > > + default:
> > > + tprintf("%d", value);
> >
> > Likewise.
> >
> > > + }
> > > + tprints("}");
> > > + }
> > > +
> > > + return RVAL_DECODED | 1;
> >
> > This shouldn't be returned on entering(tcp).
>
> If all decoding would be done when entering is finished, wouldn't it make
> sense
> to allow RVAL_DECODED here? Still don't understand how this is intended to
> work.
Usally only IOW parsers return codes with RVAL_DECODED set on entering.
IOWR also print something on exiting so they shouldn't normally return
RVAL_DECODED on entering.
--
ldv
pgp0fYbXCVwEg.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 5/5] drm: Add decoding of DRM and KMS ioctls
On Fri, Sep 11, 2015 at 01:39:29PM +0200, Patrik Jakobsson wrote:
> On Wed, Sep 09, 2015 at 01:50:40AM +0300, Dmitry V. Levin wrote:
> > On Mon, Aug 24, 2015 at 02:42:50PM +0200, Patrik Jakobsson wrote:
> > > +static int drm_mode_create_dumb(struct tcb *tcp, const unsigned int
> > > code, long arg)
> > > +{
> > > + struct drm_mode_create_dumb dumb;
> > > +
> > > + if (umove(tcp, arg, ))
> > > + return RVAL_DECODED;
> > > +
> > > + if (entering(tcp)) {
> > > + tprintf(", {width=%u, height=%u, bpp=%u, flags=0x%x",
> > > + dumb.width, dumb.height, dumb.bpp, dumb.flags);
> > > + } else if (exiting(tcp)) {
> > > + tprintf(", handle=%u, pitch=%u, size=%Lu}", dumb.handle,
> > > + dumb.pitch, dumb.size);
> > > + }
> > > +
> > > + return RVAL_DECODED | 1;
> > > +}
> >
> > This generates a warning (which turns into an error with
> > --enable-gcc-Werror) on x86_64 when using kernel drm headers:
> >
> > drm.c: In function 'drm_mode_create_dumb':
> > drm.c:521:11: error: format '%Lu' expects argument of type 'long long
> > unsigned int', but argument 4 has type 'uint64_t {aka long unsigned int}'
> > [-Werror=format=]
>
> So this brings us back to whether to include drm kernel headers or not. If
> -Werror is a requirement (which is already broken last time I checked) there
Is it? Could you cite the error, please?
> will need to be #ifdefs at various places in drm decoding. What would you
> prefer. Both options are fine by me.
This is the only place where definitions differ to the extent that it's visible.
I'd rather cast the argument to unsigned long long.
--
ldv
pgpQIVjW9KMtJ.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 5/5] drm: Add decoding of DRM and KMS ioctls
On Fri, Sep 11, 2015 at 02:20:35PM +0200, Patrik Jakobsson wrote:
> On Fri, Sep 11, 2015 at 03:10:05PM +0300, Dmitry V. Levin wrote:
> > On Fri, Sep 11, 2015 at 01:39:29PM +0200, Patrik Jakobsson wrote:
> > > On Wed, Sep 09, 2015 at 01:50:40AM +0300, Dmitry V. Levin wrote:
> > > > On Mon, Aug 24, 2015 at 02:42:50PM +0200, Patrik Jakobsson wrote:
> > > > > +static int drm_mode_create_dumb(struct tcb *tcp, const unsigned int
> > > > > code, long arg)
> > > > > +{
> > > > > + struct drm_mode_create_dumb dumb;
> > > > > +
> > > > > + if (umove(tcp, arg, ))
> > > > > + return RVAL_DECODED;
> > > > > +
> > > > > + if (entering(tcp)) {
> > > > > + tprintf(", {width=%u, height=%u, bpp=%u, flags=0x%x",
> > > > > + dumb.width, dumb.height, dumb.bpp, dumb.flags);
> > > > > + } else if (exiting(tcp)) {
> > > > > + tprintf(", handle=%u, pitch=%u, size=%Lu}", dumb.handle,
> > > > > + dumb.pitch, dumb.size);
> > > > > + }
> > > > > +
> > > > > + return RVAL_DECODED | 1;
> > > > > +}
> > > >
> > > > This generates a warning (which turns into an error with
> > > > --enable-gcc-Werror) on x86_64 when using kernel drm headers:
> > > >
> > > > drm.c: In function 'drm_mode_create_dumb':
> > > > drm.c:521:11: error: format '%Lu' expects argument of type 'long long
> > > > unsigned int', but argument 4 has type 'uint64_t {aka long unsigned
> > > > int}' [-Werror=format=]
> > >
> > > So this brings us back to whether to include drm kernel headers or not. If
> > > -Werror is a requirement (which is already broken last time I checked)
> > > there
> >
> > Is it? Could you cite the error, please?
>
> It's in aio.c:185 which at a closer look is intentional. Would still break the
> build though. But I'm not arguing that letting warnings slip through (with or
> without -Werror) is ok, just wondering if it justifies what I'm trying to do
> here.
strace is expected to build without compilation warnings assuming that the
compiler and headers are supported. In fact, I routinely build strace
with --enable-gcc-Werror. If it doesn't build for you, please report.
--
ldv
pgpAg2_5UiTmI.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 4/5] drm: Add decoding of i915 ioctls
On Tue, Sep 08, 2015 at 04:30:52AM +0300, Dmitry V. Levin wrote:
> On Tue, Sep 08, 2015 at 04:18:11AM +0300, Dmitry V. Levin wrote:
> [...]
> > So the whole function should look smth like this:
> >
> > static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> > {
> > struct drm_i915_getparam param;
> >
> > if (entering(tcp)) {
> > if (umove_or_printaddr(tcp, arg, ))
> > return RVAL_DECODED | 1;
> > tprints(", {param=");
>
> or rather
> tprints(", ");
> if (umove_or_printaddr(tcp, arg, ))
> return RVAL_DECODED | 1;
> tprints("{param=");
or rather
static int
i915_getparam(struct tcb *tcp, const unsigned int code, const long arg)
{
struct drm_i915_getparam param;
int value;
if (entering(tcp)) {
tprints(", ");
if (umove_or_printaddr(tcp, arg, ))
return RVAL_DECODED | 1;
tprints("{param=");
printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
return 0;
}
if (!umove(tcp, arg, )) {
tprints(", value=");
if (!umove_or_printaddr(tcp, (long) param.value, )) {
switch (param.param) {
case I915_PARAM_CHIPSET_ID:
tprintf("[%#04x]", value);
break;
default:
tprintf("[%d]", value);
}
}
}
tprints("}");
return 1;
}
Note that those structure members that are set by the kernel on exiting
syscall shouldn't normally be printed in case of syserror(tcp).
In case of i915_getparam, no extra check is needed because param.value is
an address supplied by userspace and umove_or_printaddr already performs
all necessary checks, but in other IOWR parsers you might want to use
if (!syserror(tcp) && !umove(tcp, arg, )) {
instead.
--
ldv
pgp9B2ipYt45y.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 5/5] drm: Add decoding of DRM and KMS ioctls
On Mon, Aug 24, 2015 at 02:42:50PM +0200, Patrik Jakobsson wrote:
> First batch of drm / kms ioctls.
Several comments in addition to issues similar to 4/5 patch.
> +static int drm_mode_rm_fb(struct tcb *tcp, const unsigned int code, long arg)
> +{
> + unsigned int handle;
> +
> +
> + if (entering(tcp)) {
> + if (umove(tcp, arg, ))
> + return RVAL_DECODED;
> +
> + tprintf(", %u", handle);
> + }
> +
> + return RVAL_DECODED | 1;
> +}
Use printnum_int instead:
tprints(", ");
printnum_int(tcp, arg, "%u");
return RVAL_DECODED | 1;
> +static int drm_mode_create_dumb(struct tcb *tcp, const unsigned int code,
> long arg)
> +{
> + struct drm_mode_create_dumb dumb;
> +
> + if (umove(tcp, arg, ))
> + return RVAL_DECODED;
> +
> + if (entering(tcp)) {
> + tprintf(", {width=%u, height=%u, bpp=%u, flags=0x%x",
> + dumb.width, dumb.height, dumb.bpp, dumb.flags);
> + } else if (exiting(tcp)) {
> + tprintf(", handle=%u, pitch=%u, size=%Lu}", dumb.handle,
> + dumb.pitch, dumb.size);
> + }
> +
> + return RVAL_DECODED | 1;
> +}
This generates a warning (which turns into an error with
--enable-gcc-Werror) on x86_64 when using kernel drm headers:
drm.c: In function 'drm_mode_create_dumb':
drm.c:521:11: error: format '%Lu' expects argument of type 'long long unsigned
int', but argument 4 has type 'uint64_t {aka long unsigned int}'
[-Werror=format=]
> @@ -112,6 +577,84 @@ int drm_ioctl(struct tcb *tcp, const unsigned int code,
> long arg)
> if (drm_is_priv(tcp->u_arg[1])) {
> if (verbose(tcp) && drm_is_driver(tcp, "i915"))
> ret = drm_i915_ioctl(tcp, code, arg);
> + } else {
> + switch (code) {
> + case DRM_IOCTL_VERSION:
> + ret = drm_version(tcp, code, arg);
> + break;
Looks like the return code can be forwarded without further delays.
--
ldv
pgpdCzNMLBccW.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 2/5] drm: Add private data field to trace control block
On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
[...]
> Here's my take on it (I assume it needs some discussion):
>
> int
> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
> {
> /* A free callback is required before setting private data and private
>* data must be set back to NULL before being set again.
>*/
I think a single function initializing both _priv_data and _free_priv_data
would suffice:
int
set_tcb_priv_data(struct tcb *tcp, void *priv_data,
void (*free_priv_data)(void *))
{
if (tcp->_priv_data)
return -1;
tcp->_free_priv_data = free_priv_data;
tcp->_priv_data = priv_data;
return 0;
}
--
ldv
pgpV4AdZ7BOOw.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 4/5] drm: Add decoding of i915 ioctls
On Mon, Aug 24, 2015 at 02:42:49PM +0200, Patrik Jakobsson wrote:
> +static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> +{
> + struct drm_i915_getparam param;
> + int value;
> +
> + if (umove(tcp, arg, ))
> + return RVAL_DECODED;
> +
> + if (entering(tcp)) {
> + tprints(", {param=");
> + printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
> + } else if (exiting(tcp)) {
> + if (umove(tcp, (long)param.value, ))
> + return RVAL_DECODED;
Since part of param has already been printed, RVAL_DECODED shouldn't be
returned here. For the same reason, RVAL_DECODED shouldn't be returned
earlier in this function.
> + tprints(", value=");
> + switch (param.param) {
> + case I915_PARAM_CHIPSET_ID:
> + tprintf("0x%04x", value);
Since the value has been fetched by address stored in param.value,
it has to be printed in brackets like in printnum_int.
> + break;
> + default:
> + tprintf("%d", value);
Likewise.
> + }
> + tprints("}");
> + }
> +
> + return RVAL_DECODED | 1;
This shouldn't be returned on entering(tcp).
> +}
So the whole function should look smth like this:
static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
{
struct drm_i915_getparam param;
if (entering(tcp)) {
if (umove_or_printaddr(tcp, arg, ))
return RVAL_DECODED | 1;
tprints(", {param=");
printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
tprints(", value=");
return 0;
} else {
int value;
if (umove(tcp, arg, )) {
tprints("???");
} else if (!umove_or_printaddr(tcp, (long) param.value,
)) {
switch (param.param) {
case I915_PARAM_CHIPSET_ID:
tprintf("[%#04x]", value);
break;
default:
tprintf("[%d]", value);
}
}
tprints("}");
return 1;
}
}
Please apply this approach to all DRM_IOWR parsers.
> +
> +static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
> +{
> + struct drm_i915_setparam param;
> +
> + if (entering(tcp)) {
> + if (umove(tcp, arg, ))
> + return RVAL_DECODED;
In this and other functions I slightly prefer
if (umove_or_printaddr(tcp, arg, ))
return RVAL_DECODED | 1;
over your variant because umove_or_printaddr() handles NULL address
and !verbose(tcp) case better.
--
ldv
pgpGBPm5dnhI6.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 3/5] drm: Add dispatcher and driver identification for DRM
On Mon, Aug 24, 2015 at 02:42:48PM +0200, Patrik Jakobsson wrote:
> * Makefile.am: Add compilation of drm.c.
> * defs.h: Add extern declaration of drm_ioctl when drm headers are found.
> * drm.c: New file.
> * ioctl.c (ioctl_decode): Dispatch drm ioctls when drm headers are found.
* defs.h (drm_decode_number, drm_ioctl): New prototypes.
* drm.c: New file.
* Makefile.am (strace_SOURCES): Add it.
* ioctl.c (ioctl_decode_command_number, ioctl_decode)
[HAVE_DRM_H || HAVE_DRM_DRM_H]: Dispatch drm ioctls.
> --- a/defs.h
> +++ b/defs.h
> @@ -612,6 +612,9 @@ extern const char *sprint_open_modes(int);
> extern void print_seccomp_filter(struct tcb *tcp, unsigned long);
>
> extern int block_ioctl(struct tcb *, const unsigned int, long);
> +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> +extern int drm_ioctl(struct tcb *, const unsigned int, long);
> +#endif
I think function these prototypes could be added unconditionally.
Note that v3 version of this patch also declared drm_decode_number().
> --- /dev/null
> +++ b/drm.c
[...]
> +#include "defs.h"
> +
> +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> +
> +#ifdef HAVE_DRM_H
> +#include
> +#else
> +#include
> +#endif
> +
> +#include
Let's include before drm stuff.
> +#define DRM_MAX_NAME_LEN 128
> +
> +inline int drm_is_priv(const unsigned int num)
> +{
> + return (_IOC_NR(num) >= DRM_COMMAND_BASE &&
> + _IOC_NR(num) < DRM_COMMAND_END);
> +}
This function has to be static, and like other static functions,
it has to be added along with its first user, otherwise the project
won't build with --enable-gcc-Werror.
> +static char *drm_get_driver_name(struct tcb *tcp)
> +{
> + char path[PATH_MAX];
> + char link[PATH_MAX];
> + int ret;
> +
> + if (getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1) < 0)
> + return NULL;
> +
> + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
> + basename(path));
if (snprintf(link, sizeof(link), ...) >= sizeof(link))
return NULL;
> +static void drm_free_priv(void *data)
> +{
> + free(data);
> +}
Do we really need a wrapper for free(3)?
> --- a/ioctl.c
> +++ b/ioctl.c
> @@ -248,6 +248,10 @@ ioctl_decode(struct tcb *tcp)
> case 0x22:
> return scsi_ioctl(tcp, code, arg);
> #endif
> +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> + case 'd':
> + return drm_ioctl(tcp, code, arg);
> +#endif
> case 'L':
> return loop_ioctl(tcp, code, arg);
> case 'M':
v3 version also patched ioctl_decode_command_number()
to call drm_decode_number().
--
ldv
pgpQ1hyYIABO8.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 4/5] drm: Add decoding of i915 ioctls
On Tue, Sep 08, 2015 at 04:18:11AM +0300, Dmitry V. Levin wrote:
[...]
> So the whole function should look smth like this:
>
> static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> {
> struct drm_i915_getparam param;
>
> if (entering(tcp)) {
> if (umove_or_printaddr(tcp, arg, ))
> return RVAL_DECODED | 1;
> tprints(", {param=");
or rather
tprints(", ");
if (umove_or_printaddr(tcp, arg, ))
return RVAL_DECODED | 1;
tprints("{param=");
> In this and other functions I slightly prefer
> if (umove_or_printaddr(tcp, arg, ))
> return RVAL_DECODED | 1;
> over your variant because umove_or_printaddr() handles NULL address
> and !verbose(tcp) case better.
And the whole function will look as simple as this:
static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
{
struct drm_i915_setparam param;
tprints(", ");
if (!umove_or_printaddr(tcp, arg, )) {
tprints("{param=");
printxval(drm_i915_setparams, param.param, "I915_PARAM_???");
tprintf(", value=%d}", param.value);
}
return RVAL_DECODED | 1;
}
Note the absence of entering(tcp) check because this DRM_IOW parser always
returns a value with RVAL_DECODED flag set.
--
ldv
pgpdDdg5mQuUu.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 1/5] drm: Add config for detecting libdrm
On Fri, Jul 31, 2015 at 11:09:11AM +0200, Patrik Jakobsson wrote: On Thu, Jul 30, 2015 at 10:04:49AM -0400, Mike Frysinger wrote: On 30 Jul 2015 15:30, Patrik Jakobsson wrote: On Thu, Jul 23, 2015 at 05:48:21AM -0400, Mike Frysinger wrote: On 01 Jul 2015 14:52, Patrik Jakobsson wrote: Use pkg-config to try to find libdrm. If that fails use the standard include directory for kernel drm headers in /usr/include/drm. * configure.ac: Use pkg-config to find libdrm Signed-off-by: Patrik Jakobsson patrik.jakobs...@linux.intel.com --- configure.ac | 4 1 file changed, 4 insertions(+) diff --git a/configure.ac b/configure.ac index bb8bf46..aa63af7 100644 --- a/configure.ac +++ b/configure.ac @@ -844,6 +844,10 @@ fi AM_CONDITIONAL([USE_LIBUNWIND], [test x$use_libunwind = xyes]) AC_MSG_RESULT([$use_libunwind]) +PKG_CHECK_MODULES([libdrm], [libdrm], + [CPPFLAGS=$CPPFLAGS $libdrm_CFLAGS], + [CPPFLAGS=$CPPFLAGS -I/usr/include/drm]) yikes, no, this is a really really bad idea. it should read: PKG_CHECK_MODULES([LIBDRM], [libdrm], [CPPFLAGS=$CPPFLAGS $LIBDRM_CFLAGS], [:]) I take it you don't want me to fallback on kernel headers and skip compiling with drm support if libdrm is not available? you cannot hardcode any path at all. if the kernel headers provide all of the defines/structs that you need, then just include them directly via #include drm/xxx.h. -mike The prefered drm way is to always use the libdrm headers and never the kernel headers. I know this is breaking the rules but it's what we got to work with. Some distros give you the kernel version and others the libdrm version. The kernel version is wrong and libdrm patches this up since we're not allowed to break the userspace interface. I think the safest way would be to only compile drm support for strace if libdrm is present and ignore the kernel headers. Unlike most of userspace, strace attempts to show the picture as it's seen from the kernel perspective. Sometimes it forces us to use kernel headers instead of headers provided by libc and other libraries. If kernel itself uses uapi/drm, it should be safe for strace to use it as well. I suppose the check could be written this way: PKG_CHECK_MODULES([LIBDRM], [libdrm], [CPPFLAGS=$CPPFLAGS $LIBDRM_CFLAGS AC_CHECK_HEADERS([drm.h i915_drm.h])], [AC_CHECK_HEADERS([drm/drm.h drm/i915_drm.h])]) ... #if defined HAVE_DRM_H || defined HAVE_DRM_DRM_H # ifdef HAVE_DRM_H # include drm.h # else # include drm/drm.h # endif [rest of drm.c] #endif /* HAVE_DRM_H || HAVE_DRM_DRM_H */ -- ldv pgpD2LusmvyQ4.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 1/5] drm: Add config for detecting libdrm
On Thu, Jul 23, 2015 at 05:48:21AM -0400, Mike Frysinger wrote: On 01 Jul 2015 14:52, Patrik Jakobsson wrote: Use pkg-config to try to find libdrm. If that fails use the standard include directory for kernel drm headers in /usr/include/drm. * configure.ac: Use pkg-config to find libdrm Signed-off-by: Patrik Jakobsson patrik.jakobs...@linux.intel.com --- configure.ac | 4 1 file changed, 4 insertions(+) diff --git a/configure.ac b/configure.ac index bb8bf46..aa63af7 100644 --- a/configure.ac +++ b/configure.ac @@ -844,6 +844,10 @@ fi AM_CONDITIONAL([USE_LIBUNWIND], [test x$use_libunwind = xyes]) AC_MSG_RESULT([$use_libunwind]) +PKG_CHECK_MODULES([libdrm], [libdrm], + [CPPFLAGS=$CPPFLAGS $libdrm_CFLAGS], + [CPPFLAGS=$CPPFLAGS -I/usr/include/drm]) yikes, no, this is a really really bad idea. it should read: PKG_CHECK_MODULES([LIBDRM], [libdrm], [CPPFLAGS=$CPPFLAGS $LIBDRM_CFLAGS], [:]) Why [:] ? Wouldn't [] suffice? -- ldv pgpR2DMrJSFVq.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/5] drm: Add decoding of i915 ioctls
On Fri, Jul 10, 2015 at 02:36:38PM +0200, Patrik Jakobsson wrote: On Wed, Jul 08, 2015 at 03:11:36AM +0300, Dmitry V. Levin wrote: On Mon, Jul 06, 2015 at 04:40:24PM +0200, Gabriel Laskar wrote: [...] Anyway, SYS_FUNC(ioctl) is a bit complicated, and the handling of the fallbacks on failure should be more generic. What would be useful is a way for on entering parsers to return done with decoding information to their callers. This could be implemented by or'ing return value in the current semantics with a flag with done with decoding meaning, e.g. RVAL_DONE. If an ioctl parser returned RVAL_DONE, this would tell SYS_FUNC(ioctl) that the decoding is finished but fallback decoding is needed, while RVAL_DONE+1 would mean that the decoding is finished and no fallback decoding is needed. I like that idea but isn't the current return semantics already good enough for that? The problem right now is that we ignore the return value from ioctl_decode() on entering. After commit v4.10-104-g204c2bc we no longer ignore the return value from ioctl_decode() on entering. -- ldv pgpimtomM5mde.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/5] drm: Add decoding of i915 ioctls
On Mon, Jul 06, 2015 at 04:40:24PM +0200, Gabriel Laskar wrote:
On Mon, 6 Jul 2015 12:35:52 +0200, Patrik Jakobsson wrote:
On Fri, Jul 03, 2015 at 03:36:09AM +0300, Dmitry V. Levin wrote:
On Wed, Jul 01, 2015 at 02:52:47PM +0200, Patrik Jakobsson wrote:
[...]
--- a/drm.c
+++ b/drm.c
@@ -35,6 +35,9 @@
#define DRM_MAX_NAME_LEN 128
+extern int drm_i915_decode_number(struct tcb *tcp, unsigned int arg);
Please rename arg to code, and ...
+extern int drm_i915_ioctl(struct tcb *tcp, const unsigned int code,
long arg);
... move both declarations to defs.h to make them visible also
in the file where these functions are defined.
[...]
+static int i915_setparam(struct tcb *tcp, const unsigned int code,
long arg)
+{
+ struct drm_i915_setparam param;
+
+ if (entering(tcp)) {
+ if (umove(tcp, arg, param))
+ return 0;
+
+ tprints(, {param=);
+ printxval(drm_i915_setparams, param.param,
I915_PARAM_???);
+ tprintf(, value=%d}, param.value);
+ }
+
+ return 1;
+}
In this and most of other parsers of _IOC_WRITE ioctls added by this and
the next patches, any error in parser that leads to return 0 will result
to disabled arg decoding, including the fallback decoding performed by
sys_ioctl.
Maybe it's time to deal with this issue in a more generic way.
Yes, I'm thinking SYS_FUNC(ioctl) could be improved. But on the other hand
how
likely is it that we fail in umove and what chance do we have to recover
from
that anyway? All I can think of is OOM.
umove() can fail in multiple ways. For example, if the memory is not
valid in the tracee, umove() will fail.
Yes, this is the most likely cause for umove() to fail,
and the most easily reproducible one, e.g.
ioctl(-1, DRM_IOCTL_VERSION, 42);
Anyway, SYS_FUNC(ioctl) is a bit complicated, and the handling of the
fallbacks on failure should be more generic.
What would be useful is a way for on entering parsers to return
done with decoding information to their callers.
This could be implemented by or'ing return value in the current semantics
with a flag with done with decoding meaning, e.g. RVAL_DONE.
If an ioctl parser returned RVAL_DONE, this would tell SYS_FUNC(ioctl)
that the decoding is finished but fallback decoding is needed, while
RVAL_DONE+1 would mean that the decoding is finished and no fallback
decoding is needed.
--
ldv
pgpCb97OU3bZW.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/5] drm: Add private data field to trace control block
On Wed, Jul 01, 2015 at 02:52:45PM +0200, Patrik Jakobsson wrote:
[...]
--- a/defs.h
+++ b/defs.h
@@ -266,6 +266,13 @@ struct tcb {
int u_error;/* Error code */
long scno; /* System call number */
long u_arg[MAX_ARGS]; /* System call arguments */
+
+ /*
+ * Private data for the decoding functions of the syscall. TCB core does
+ * _not_ handle allocation / deallocation of this data.
+ */
+ void *priv_data;
+
This will result to memory leaks if droptcb() is called before the
syscall parser that allocated memory had a chance to deallocate it.
As this data is no longer relevant after leaving trace_syscall_exiting(),
I suggest to perform deallocation directly from trace_syscall_exiting.
This API could be made more flexible by adding another pointer -
the function to be called to deallocate memory, e.g.
struct tcb {
...
void *priv_data;
void (*free_priv_data)(void *);
...
};
...
void
free_priv_data(struct tcb *tcp)
{
if (tcp-priv_data) {
if (tcp-free_priv_data) {
tcp-free_priv_data(tcp-priv_data);
tcp-free_priv_data = NULL;
}
tcp-priv_data = NULL;
}
}
...
droptcb(struct tcb *tcp)
{
...
free_priv_data(tcp);
...
}
...
trace_syscall_exiting(struct tcb *tcp)
{
...
ret:
free_priv_data(tcp);
...
}
[...]
On Wed, Jul 01, 2015 at 02:52:46PM +0200, Patrik Jakobsson wrote:
* Makefile.am: Add compilation of drm.c
* defs.h: Declarations of drm functions
* drm.c: Utility functions for drm driver detection
* io.c: Dispatch drm ioctls
* ioctl.c: Distpatch generic and driver specific ioctls
This is not quite a GNU style changelog entry. Please have a look at
http://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html
and examples in strace.git history.
[...]
+#include defs.h
+
+#include drm.h
+#include linux/limits.h
Please include sys/param.h instead of linux/limits.h.
+#define DRM_MAX_NAME_LEN 128
+
+struct drm_ioctl_priv {
+ char name[DRM_MAX_NAME_LEN];
+};
+
+inline int drm_is_priv(const unsigned int num)
+{
+ return (_IOC_NR(num) = DRM_COMMAND_BASE
+ _IOC_NR(num) DRM_COMMAND_END);
+}
+
+static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize)
+{
+ char path[PATH_MAX];
+ char link[PATH_MAX];
+ int ret;
+
+ ret = getfdpath(tcp, tcp-u_arg[0], path, PATH_MAX - 1);
+ if (ret 0)
+ return ret;
+
+ snprintf(link, PATH_MAX, /sys/class/drm/%s/device/driver,
+ basename(path));
+
+ ret = readlink(link, path, PATH_MAX - 1);
+ if (ret 0)
+ return ret;
+
+ path[ret] = '\0';
+ strncpy(name, basename(path), bufsize);
+
+ return 0;
+}
I think this is getting too complicated. This function could just return
strdup(basename(path)) or NULL in case of any error:
static char *
drm_get_driver_name(struct tcb *tcp, const char *name)
{
char path[PATH_MAX];
char link[PATH_MAX];
int ret;
if (getfdpath(tcp, tcp-u_arg[0], path, PATH_MAX - 1) 0)
return NULL;
if (snprintf(link, PATH_MAX, /sys/class/drm/%s/device/driver,
basename(path)) = PATH_MAX)
return NULL;
ret = readlink(link, path, PATH_MAX - 1);
if (ret 0)
return NULL;
path[ret] = '\0';
return strdup(basename(path));
}
+
+int drm_is_driver(struct tcb *tcp, const char *name)
+{
+ struct drm_ioctl_priv *priv;
+ int ret;
+
+ /*
+ * If no private data is allocated we are detecting the driver name for
+ * the first time and must resolve it.
+ */
+ if (tcp-priv_data == NULL) {
+ tcp-priv_data = xcalloc(1, sizeof(struct drm_ioctl_priv));
xcalloc shouldn't be used if a potential memory allocation error is not
fatal. In a parser that performs verbose syscall decoding no memory
allocation error is fatal.
+ priv = tcp-priv_data;
+
+ ret = drm_get_driver_name(tcp, priv-name, DRM_MAX_NAME_LEN);
+ if (ret)
+ return 0;
+ }
+
+ priv = tcp-priv_data;
+
+ return strncmp(name, priv-name, DRM_MAX_NAME_LEN) == 0;
Then with priv_data+free_priv_data interface this would looks smth like
...
if (!tcp-priv_data) {
tcp-priv_data = drm_get_driver_name(tcp, name);
if (tcp-priv_data) {
tcp-free_priv_data = free;
} else {
tcp-priv_data = (void *) ;
tcp-free_priv_data = NULL;
}
}
return !strcmp(name, (char *) tcp-priv_data);
+}
+
+int drm_decode_number(struct tcb *tcp, unsigned int arg)
This is an ioctl request code, let's
Re: [Intel-gfx] [PATCH v3 4/5] drm: Add decoding of i915 ioctls
On Wed, Jul 01, 2015 at 02:52:47PM +0200, Patrik Jakobsson wrote:
[...]
--- a/drm.c
+++ b/drm.c
@@ -35,6 +35,9 @@
#define DRM_MAX_NAME_LEN 128
+extern int drm_i915_decode_number(struct tcb *tcp, unsigned int arg);
Please rename arg to code, and ...
+extern int drm_i915_ioctl(struct tcb *tcp, const unsigned int code, long
arg);
... move both declarations to defs.h to make them visible also
in the file where these functions are defined.
[...]
+static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
+{
+ struct drm_i915_setparam param;
+
+ if (entering(tcp)) {
+ if (umove(tcp, arg, param))
+ return 0;
+
+ tprints(, {param=);
+ printxval(drm_i915_setparams, param.param, I915_PARAM_???);
+ tprintf(, value=%d}, param.value);
+ }
+
+ return 1;
+}
In this and most of other parsers of _IOC_WRITE ioctls added by this and
the next patches, any error in parser that leads to return 0 will result
to disabled arg decoding, including the fallback decoding performed by
sys_ioctl.
Maybe it's time to deal with this issue in a more generic way.
--
ldv
pgpe2raSsngWM.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm: Add dispatcher and driver identification for DRM
On Sun, Jun 14, 2015 at 01:12:45PM +0200, Patrik Jakobsson wrote: [...] How about adding a void *private field to struct tcb. That way any syscall can store additional data across the life of the tcb. We can add a field to struct tcb, but its semantics wrt memory management should be strictly defined. For example, who is responsible for memory deallocation, what droptcb() should do about this field, etc. In this case, we probably need to store a pointer to a string dynamically allocated in drm_get_driver_name() (on entering syscall) which is automatically deallocated later in trace_syscall_exiting() and droptcb(). -- ldv pgprsenA7A0jK.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm: Add dispatcher and driver identification for DRM
On Thu, Jun 11, 2015 at 04:11:49PM +0200, Patrik Jakobsson wrote:
On Thu, Jun 11, 2015 at 02:26:59AM +0300, Dmitry V. Levin wrote:
On Wed, Jun 10, 2015 at 01:52:33PM +0200, Patrik Jakobsson wrote:
On Wed, Jun 10, 2015 at 01:14:20AM +0300, Dmitry V. Levin wrote:
On Tue, Jun 09, 2015 at 01:26:42PM +0200, Patrik Jakobsson wrote:
[...]
+#define DRM_MAX_NAME_LEN 128
+
+inline int drm_is_priv(const unsigned int num)
+{
+ return (_IOC_NR(num) = DRM_COMMAND_BASE
+ _IOC_NR(num) DRM_COMMAND_END);
+}
+
+static int drm_get_driver_name(struct tcb *tcp, char *name, size_t
bufsize)
+{
+ char path[PATH_MAX];
+ char link[PATH_MAX];
+ int ret;
+
+ ret = getfdpath(tcp, tcp-u_arg[0], path, PATH_MAX - 1);
+ if (!ret)
+ return ret;
+
+ snprintf(link, PATH_MAX, /sys/class/drm/%s/device/driver,
+ basename(path));
+
+ ret = readlink(link, path, PATH_MAX - 1);
+ if (ret 0)
+ return ret;
+
+ path[ret] = '\0';
+ strncpy(name, basename(path), bufsize);
+
+ return 0;
+}
+
+int drm_is_driver(struct tcb *tcp, const char *name)
+{
+ char drv[DRM_MAX_NAME_LEN];
+ int ret;
+
+ ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN);
+ if (ret)
+ return 0;
+
+ return strcmp(name, drv) == 0;
+}
This interface will result to several getfdpath() calls per
ioctl_decode(). If the only purpose of drm_is_driver() is to help
finding
the most appropriate function, let's create a table of pairs
{driver name, function} and pass this table to a function that will do a
single getfdpath() call, calculate the driver name, and choose the right
function from the table.
Yes I was thinking the same thing but it's a bit tricky. What I need is:
fd - path - driver name. And fd - path could change between ioctls. It
is not
a likely scenario but it's possible. I could get rid of the extra call in
drm_decode_number() if I put it back in drm_ioctl as in my RFC. I could
also
optimize path - driver name with a table but I don't know how expensive
those
calls actually are. Not sure what would be the best solution here.
drm_get_driver_name() is quite expensive as it does two readlink syscalls,
so it should be called at most once per ioctl_decode().
Another method to achieve this is to change drm_get_driver_name() to return
basename(path) instead of return code, so that drm_ioctl() would call it
once and pass the result to strcmp calls:
int
drm_ioctl(struct tcb *tcp, const unsigned int code, long arg)
{
if (verbose(tcp) drm_is_priv(code)) {
const char *driver = drm_get_driver_name(tcp);
if (!driver)
return 0;
if (!strcmp(driver, i915))
return drm_i915_ioctl(tcp, code, arg);
}
return 0;
}
I misunderstood you. As you say, we shouldn't do a drm_get_driver_name() more
than once (no matter how many drm drivers we are looking for) so your
suggestion
above would fix that. I was thinking about how to get rid of the extra call in
drm_decode_number() (if we could somehow squash them together). But that would
make things rather ugly. If ok with you we could just have the same approach
in
drm_decode_number() as above and live with the fact that we get two calls to
drm_get_driver_name() per DRM device specific ioctl. One for
drm_decode_number()
and one for drm_ioctl().
This way we would end up with three drm_get_driver_name() calls per ioctl:
twice on entering syscall and once on exiting. Maybe we could cache
result of the first of these three calls somewhere?
--
ldv
pgpK6Q3_uykrE.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm: Add decoding of i915 ioctls
On Thu, Jun 11, 2015 at 03:34:14PM +0200, Patrik Jakobsson wrote:
On Thu, Jun 11, 2015 at 02:27:12AM +0300, Dmitry V. Levin wrote:
On Wed, Jun 10, 2015 at 02:45:24PM +0200, Patrik Jakobsson wrote:
On Wed, Jun 10, 2015 at 01:35:35AM +0300, Dmitry V. Levin wrote:
On Tue, Jun 09, 2015 at 01:26:43PM +0200, Patrik Jakobsson wrote:
[...]
+static int i915_setparam(struct tcb *tcp, const unsigned int code,
long arg)
+{
+ struct drm_i915_setparam param;
+
+ if (exiting(tcp) || umove(tcp, arg, param))
+ return 0;
In this and other ioctl printers that unconditionally return 0 on exit,
wouldn't the caller treat it as an ioctl that hasn't been printed?
Yes, seems like the exiting phase should return 1 if already handled in
the
entering phase. But changing it produces the same output for some reason.
Not
sure what's going on here.
Isn't tcp-u_arg[2] printed twice, the first time decoded,
and the second time in hex?
My mistake, it does print u_arg[2] in hex as well. Luckily they could all be
moved to the exiting phase instead. Fixed in v2.
The common rule is to decode as early as possible, consequently, all
syscall arguments that could be decoded on entering syscall should be
decoded on entering rather than on exiting. All syscall arguments
of _IOW ioctl commands could be fully decoded on entering syscall.
--
ldv
pgp2D72UvJixD.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm: Add decoding of i915 ioctls
On Wed, Jun 10, 2015 at 02:45:24PM +0200, Patrik Jakobsson wrote:
On Wed, Jun 10, 2015 at 01:35:35AM +0300, Dmitry V. Levin wrote:
On Tue, Jun 09, 2015 at 01:26:43PM +0200, Patrik Jakobsson wrote:
[...]
+static int i915_setparam(struct tcb *tcp, const unsigned int code, long
arg)
+{
+ struct drm_i915_setparam param;
+
+ if (exiting(tcp) || umove(tcp, arg, param))
+ return 0;
In this and other ioctl printers that unconditionally return 0 on exit,
wouldn't the caller treat it as an ioctl that hasn't been printed?
Yes, seems like the exiting phase should return 1 if already handled in the
entering phase. But changing it produces the same output for some reason. Not
sure what's going on here.
Isn't tcp-u_arg[2] printed twice, the first time decoded,
and the second time in hex?
--
ldv
pgpEIy0KojEQe.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm: Add dispatcher and driver identification for DRM
On Wed, Jun 10, 2015 at 01:52:33PM +0200, Patrik Jakobsson wrote:
On Wed, Jun 10, 2015 at 01:14:20AM +0300, Dmitry V. Levin wrote:
On Tue, Jun 09, 2015 at 01:26:42PM +0200, Patrik Jakobsson wrote:
[...]
+#define DRM_MAX_NAME_LEN 128
+
+inline int drm_is_priv(const unsigned int num)
+{
+ return (_IOC_NR(num) = DRM_COMMAND_BASE
+ _IOC_NR(num) DRM_COMMAND_END);
+}
+
+static int drm_get_driver_name(struct tcb *tcp, char *name, size_t
bufsize)
+{
+ char path[PATH_MAX];
+ char link[PATH_MAX];
+ int ret;
+
+ ret = getfdpath(tcp, tcp-u_arg[0], path, PATH_MAX - 1);
+ if (!ret)
+ return ret;
+
+ snprintf(link, PATH_MAX, /sys/class/drm/%s/device/driver,
+ basename(path));
+
+ ret = readlink(link, path, PATH_MAX - 1);
+ if (ret 0)
+ return ret;
+
+ path[ret] = '\0';
+ strncpy(name, basename(path), bufsize);
+
+ return 0;
+}
+
+int drm_is_driver(struct tcb *tcp, const char *name)
+{
+ char drv[DRM_MAX_NAME_LEN];
+ int ret;
+
+ ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN);
+ if (ret)
+ return 0;
+
+ return strcmp(name, drv) == 0;
+}
This interface will result to several getfdpath() calls per
ioctl_decode(). If the only purpose of drm_is_driver() is to help finding
the most appropriate function, let's create a table of pairs
{driver name, function} and pass this table to a function that will do a
single getfdpath() call, calculate the driver name, and choose the right
function from the table.
Yes I was thinking the same thing but it's a bit tricky. What I need is:
fd - path - driver name. And fd - path could change between ioctls. It is
not
a likely scenario but it's possible. I could get rid of the extra call in
drm_decode_number() if I put it back in drm_ioctl as in my RFC. I could also
optimize path - driver name with a table but I don't know how expensive those
calls actually are. Not sure what would be the best solution here.
drm_get_driver_name() is quite expensive as it does two readlink syscalls,
so it should be called at most once per ioctl_decode().
Another method to achieve this is to change drm_get_driver_name() to return
basename(path) instead of return code, so that drm_ioctl() would call it
once and pass the result to strcmp calls:
int
drm_ioctl(struct tcb *tcp, const unsigned int code, long arg)
{
if (verbose(tcp) drm_is_priv(code)) {
const char *driver = drm_get_driver_name(tcp);
if (!driver)
return 0;
if (!strcmp(driver, i915))
return drm_i915_ioctl(tcp, code, arg);
}
return 0;
}
--
ldv
pgpy4jMxtLaWP.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm: Add decoding of i915 ioctls
On Tue, Jun 09, 2015 at 01:26:43PM +0200, Patrik Jakobsson wrote:
[...]
+static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
+{
+ struct drm_i915_getparam param;
+ int value;
+
+ if (entering(tcp) || umove(tcp, arg, param))
+ return 0;
+ if (umove(tcp, (long)param.value, value))
+ return 0;
+
+ tprintf(, {param=);
We use tprints to print regular strings.
+static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
+{
+ struct drm_i915_setparam param;
+
+ if (exiting(tcp) || umove(tcp, arg, param))
+ return 0;
In this and other ioctl printers that unconditionally return 0 on exit,
wouldn't the caller treat it as an ioctl that hasn't been printed?
--
ldv
pgp5rnp2hAveW.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm: Add decoding of DRM and KMS ioctls
On Tue, Jun 09, 2015 at 01:26:44PM +0200, Patrik Jakobsson wrote:
[...]
+static int drm_version(struct tcb *tcp, const unsigned int code, long arg)
+{
+ struct drm_version ver;
+ char *name, *date, *desc;
+ int ret;
+
+ if (entering(tcp) || umove(tcp, arg, ver))
+ return 0;
+
+ name = calloc(ver.name_len + 1, 1);
+ if (!name)
+ return 0;
+ ret = umovestr(tcp, (long)ver.name, ver.name_len, name);
+ if (ret 0)
+ goto free_name;
No need to allocate (arbitrary sized chunk of) memory just to fetch
a string that has to be printed. There is a function called printstr()
to print such strings in a properly quoted form.
--
ldv
pgp0kBBLAKjpZ.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm: Add decoding of DRM and KMS ioctls
On Tue, Jun 09, 2015 at 04:38:40PM +0200, Gabriel Laskar wrote:
On Tue, 9 Jun 2015 16:29:31 +0200
Patrik Jakobsson patrik.jakobs...@linux.intel.com wrote:
On Tue, Jun 09, 2015 at 03:51:08PM +0200, Gabriel Laskar wrote:
On Tue, 9 Jun 2015 13:26:44 +0200
Patrik Jakobsson patrik.jakobs...@linux.intel.com wrote:
This patch adds many of the DRM and KMS ioctls. The rest can be added as
needed.
Signed-off-by: Patrik Jakobsson patrik.jakobs...@linux.intel.com
---
drm.c | 519
++
1 file changed, 519 insertions(+)
diff --git a/drm.c b/drm.c
index fa98fb7..e550c34 100644
--- a/drm.c
+++ b/drm.c
@@ -82,6 +82,468 @@ int drm_is_driver(struct tcb *tcp, const char *name)
return strcmp(name, drv) == 0;
}
+static int drm_version(struct tcb *tcp, const unsigned int code, long
arg)
+{
+ struct drm_version ver;
+ char *name, *date, *desc;
+ int ret;
+
+ if (entering(tcp) || umove(tcp, arg, ver))
+ return 0;
+
+ name = calloc(ver.name_len + 1, 1);
We have some wrappers for that now, you can call xcalloc(), but it will
die if this does not work. Your version have the advantage of not kill
strace, and just not decode the argument.
If ok I'll keep it as calloc? As you say, dying here is not really
neccessary.
Yeah, that was mostly me thinking out loud. Imho, I would keep the
calloc(), but I don't know what Dmitry will prefer. Maybe for
consistency, it should be better to have an xcalloc().
The general rule is to call xcalloc() if die_out_of_memory() is the best
way to handle the OOM condition. strace should neither attempt to
allocate arbitrary large chunks of memory specified by user input
nor die when such allocation requests fail.
--
ldv
pgpT0lLEITVSL.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm: Add dispatcher and driver identification for DRM
On Tue, Jun 09, 2015 at 01:26:42PM +0200, Patrik Jakobsson wrote:
[...]
+static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize)
+{
+ char path[PATH_MAX];
+ char link[PATH_MAX];
+ int ret;
+
+ ret = getfdpath(tcp, tcp-u_arg[0], path, PATH_MAX - 1);
+ if (!ret)
+ return ret;
The return code of getfdpath() is essentially the return code of
readlink(), so the check should be the same as after readlink() below.
+
+ snprintf(link, PATH_MAX, /sys/class/drm/%s/device/driver,
+ basename(path));
+
+ ret = readlink(link, path, PATH_MAX - 1);
+ if (ret 0)
+ return ret;
+
+ path[ret] = '\0';
+ strncpy(name, basename(path), bufsize);
+
+ return 0;
+}
--
ldv
pgptJdMY46ej4.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm: Add dispatcher and driver identification for DRM
On Tue, Jun 09, 2015 at 01:26:42PM +0200, Patrik Jakobsson wrote:
[...]
--- a/Makefile.am
+++ b/Makefile.am
@@ -121,6 +121,7 @@ strace_SOURCES = \
utime.c \
utimes.c\
v4l2.c \
+ drm.c \
vsprintf.c \
wait.c \
xattr.c
Starting with v4.7-166-g7ae73a9, we keep strace_SOURCES list sorted.
--- /dev/null
+++ b/drm.c
[...]
+#include defs.h
+
+#include sys/types.h
+#include unistd.h
+#include string.h
+#include linux/limits.h
+#include stdint.h
+#include sys/ioctl.h
+#include linux/types.h
+#include drm.h
No need to include header files already included by defs.h.
Most likely no need to include linux/limits.h or linux/types.h.
+#define DRM_MAX_NAME_LEN 128
+
+inline int drm_is_priv(const unsigned int num)
+{
+ return (_IOC_NR(num) = DRM_COMMAND_BASE
+ _IOC_NR(num) DRM_COMMAND_END);
+}
+
+static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize)
+{
+ char path[PATH_MAX];
+ char link[PATH_MAX];
+ int ret;
+
+ ret = getfdpath(tcp, tcp-u_arg[0], path, PATH_MAX - 1);
+ if (!ret)
+ return ret;
+
+ snprintf(link, PATH_MAX, /sys/class/drm/%s/device/driver,
+ basename(path));
+
+ ret = readlink(link, path, PATH_MAX - 1);
+ if (ret 0)
+ return ret;
+
+ path[ret] = '\0';
+ strncpy(name, basename(path), bufsize);
+
+ return 0;
+}
+
+int drm_is_driver(struct tcb *tcp, const char *name)
+{
+ char drv[DRM_MAX_NAME_LEN];
+ int ret;
+
+ ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN);
+ if (ret)
+ return 0;
+
+ return strcmp(name, drv) == 0;
+}
This interface will result to several getfdpath() calls per
ioctl_decode(). If the only purpose of drm_is_driver() is to help finding
the most appropriate function, let's create a table of pairs
{driver name, function} and pass this table to a function that will do a
single getfdpath() call, calculate the driver name, and choose the right
function from the table.
[...]
@@ -284,6 +294,7 @@ ioctl_decode(struct tcb *tcp, unsigned int code, long arg)
* d sys/des.h (possible overlap)
* d vax/dkio.h (possible overlap)
* d vaxuba/rxreg.h (possible overlap)
+ * d drm/drm.h
* f sys/filio.h
* g sunwindow/win_ioctl.h -no overlap-
* g sunwindowdev/winioctl.c !no manifest constant! -no
overlap-
This is a history, no need to patch it.
--
ldv
pgpR95kJajcmt.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 0/2] strace/drm: Add i915 ioctls to strace
On Tue, May 12, 2015 at 07:37:59PM +0200, Gabriel Laskar wrote: On Tue, 12 May 2015 14:35:28 +0200, Patrik Jakobsson wrote: On Mon, May 11, 2015 at 08:08:19PM +0200, Gabriel Laskar wrote: On Mon, 11 May 2015 15:54:24 +0200, Patrik Jakobsson wrote: On Mon, May 11, 2015 at 12:50:36PM +0200, Gabriel Laskar wrote: On Wed, 6 May 2015 16:48:01 +0200, Patrik Jakobsson wrote: This patch set aims to make strace more useful when tracing i915 ioctls. The ioctl type is first checked for being drm and then the driver backing the opened device is identified by looking at sysfs. Other drivers than i915 can easily be added. Only a subset of the i915 ioctls are included. I will extend this patch set if the approach looks ok. The generic drm ioctls are also missing. Give it a spin with: strace -e trace=ioctl -p `pidof X` Patrik Jakobsson (2): strace/drm: Print extended info for drm and i915 ioctls strace/drm: Print args for most common i915 ioctls Makefile.am| 2 + defs.h | 2 + drm.c | 104 + drm_i915.c | 278 + ioctl.c| 5 + xlat/drm_i915_getparams.in | 28 + xlat/drm_i915_ioctls.in| 51 + xlat/drm_i915_setparams.in | 4 + 8 files changed, 474 insertions(+) create mode 100644 drm.c create mode 100644 drm_i915.c create mode 100644 xlat/drm_i915_getparams.in create mode 100644 xlat/drm_i915_ioctls.in create mode 100644 xlat/drm_i915_setparams.in This is a great start! We need this kind of decoding. Do you plan to add also the generic drm ioctl decoding? Thanks for the review. Yes, my plan is to add generic drm ioctls as well. Some issues though: * The way you avoid the ioctl request decoding is quite hard to follow, but it seems that you don't have much of a choice, except that in drm_ioctl(), the code from SYS_FUNC(ioctl) is duplicated. It seems it needs some work here, to allow a simpler code path. Maybe this would be clearer if the decoding/drm_get_driver_name, etc… was in ioctl_decode_command_number(). Also, with the actual code, if you are on i915 with an invalid ioctl number, it will be printed as I915_IOCTL_??? and not _IOC(...) (see below for an example.) This will also add an inconsistent result depending whether /sys is mounted or not. Yes, moving it to ioctl_decode_command_number() makes sense. I'll do that. And I'll make the output consistent and skip the I915_IOCTL_???. It comes with the drawback of possibly duplicated entries when doing the lookup even though we know we're talking to i915, but it is still nicer than _???. If you call all the request decoding code from ioctl_decode_command_number() you will still be able to determine if you are on i915, and write the correct request, but the fallback code will be no longer duplicated. You will have to call xlookup() instead of printxval(), in order to be able to know when the decoding fail though. Unfortunately I cannot check for i915 in _decode_command_number since I don't have the full tcb context (cannot figure out the path, etc.). That's probably why I had to duplicate the decoding in the first place. The only other solution I see is to detect i915 early and store it globally somewhere but that is rather nasty. Not sure how to do this in a better way. What am I missing? Oh yes, I see. We can add tcb context to ioctl_decode_command_number(), it should not be a problem. We didn't have the need for it for the moment, that's all. Dmitry? What do you think of this? I had no chance to have a look at the code, but anyway, passing tcb context down to ioctl_decode_command_number() shouldn't be a problem at all. -- ldv ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
