Re: [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 pgpxxoL2F9ZJ7.pgp Description: PGP signature -- Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/___ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
Re: [PATCH v3 4/5] drm: Add decoding of i915 ioctls
On Thu, Jul 09, 2015 at 05:19:33AM +0300, Dmitry V. Levin 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: > > > 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: > [...] > > > > > Maybe it's time to deal with this issue in a more generic way. > [...] > > > 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've pushed this and other related changes along with some usage examples. I'll have a look. Thanks. > > > -- > ldv > -- > Don't Limit Your Business. Reach for the Cloud. > GigeNET's Cloud Solutions provide you with the tools and support that > you need to offload your IT needs and focus on growing your business. > Configured For All Businesses. Start Your Cloud Today. > https://www.gigenetcloud.com/ > ___ > Strace-devel mailing list > Strace-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/strace-devel -- Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/ ___ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
Re: [PATCH v3 4/5] drm: Add decoding of i915 ioctls
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:
> > 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, ¶m))
> > > > > + 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);
Yes, then we definitely need to handle those fails better
>
> > 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". What we could do is:
1. Call ioctl_decode_number()
2. Call ioctl_decode() if above didn't fail
3. If any of 1 and/or 2 failed we do fallback
>
>
> --
> ldv
--
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
___
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel
Re: [PATCH v3 4/5] drm: Add decoding of i915 ioctls
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: > > 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: [...] > > > > Maybe it's time to deal with this issue in a more generic way. [...] > > 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've pushed this and other related changes along with some usage examples. -- ldv pgpcp2y0RBvWV.pgp Description: PGP signature -- Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/___ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
Re: [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, ¶m))
> > > > + 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
pgpvjvUdXv3sO.pgp
Description: PGP signature
--
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/___
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel
Re: [PATCH v3 4/5] drm: Add decoding of i915 ioctls
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, ¶m))
> > > + 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.
Anyway, SYS_FUNC(ioctl) is a bit complicated, and the handling of the
fallbacks on failure should be more generic.
--
Gabriel Laskar
--
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
___
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel
Re: [PATCH v3 4/5] drm: Add decoding of i915 ioctls
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, ¶m))
> > + 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.
>
> --
> ldv
--
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
___
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel
Re: [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, ¶m))
> + 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
pgpwILi8UU7cU.pgp
Description: PGP signature
--
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/___
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel
