[RFC PATCH v1 2/3] drm: implement decoding of DRM ioctls

Zhibin Li haoyouab at gmail.com
Sun Jun 16 12:56:54 UTC 2019


On Thu, Jun 13, 2019 at 10:38 PM Dmitry V. Levin <ldv at altlinux.org> wrote:

> On Thu, Jun 13, 2019 at 12:47:19AM +0800, Zhibin Li wrote:
> [...]
> > > > +#ifdef HAVE_DRM_H
> > > > +# include <drm.h>
> > > > +#else
> > > > +# include <drm/drm.h>
> > > > +#endif
> > >
> > > This means that either <drm.h> or <drm/drm.h> is always available.
> > > Can we assume this?
> >
> > I think so, too. But here's what Patrik said about it:
> > DRM is a bit tricky in this regard. All userspace applications should go
> > through libdrm and never access ioctls directly. Some distributions even
> > remove the drm headers from their kernel headers package to prevent any
> > misuse.
> > Will there be a situation that drm headers is removed while libdrm is
> > absent? In such case they are not available. I'm not so sure about this.
>
> libdrm headers are not necessarily installed, so if kernel headers do not
> provide drm headers, then neither <drm.h> nor <drm/drm.h> is available.
>
> We should handle this situation gracefully, either ifdef the whole bulk
> of drm parsers or bail out at configure stage.
>
> When I try to ifdef the whole bulk of drm parsers, everything goes well in
drm.c
but in drm_mpers.c, I always get errors like:

mpers-mx32/struct_drm_version.c:7: error: unterminated #if
    7 | #if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
      |
mpers-m32/struct_drm_version.c:7: error: unterminated #if
    7 | #if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)

I guess something goes wrong in the mpers code because the last #endif is
missing.
Any clues what might be the cause?

> > > +
> > > > +#define PRINT_IOWR(nr, size, str) \
> > > > + tprintf("DRM_IOWR(%#x, %#x) /* %s */", nr, size, str)
> > >
> > > What's the benefit of making it a macro instead of a function?
> >
> > I'm not sure if there's any. Any suggestions?
>
> If there is no benefit of making it a macro, let it be a function.
>
> > > > +int
> > > > +drm_decode_number(struct tcb *const tcp, const unsigned int code)
> > > > +{
> > > > + const unsigned int nr = _IOC_NR(code);
> > > > +
> > > > + if (_IOC_DIR(code) == (_IOC_READ | _IOC_WRITE)) {
> > > > +         switch (nr) {
> > > > +                 case 0xa7:
> > > > +                         if (_IOC_SIZE(code) != 0x50) {
> > > > +                                 PRINT_IOWR(nr, _IOC_SIZE(code),
> > > > +
> "DRM_IOCTL_MODE_GETCONNECTOR");
> > > > +                                 return IOCTL_NUMBER_STOP_LOOKUP;
> > >
> > > What is 0x50?  Why do we skip lookup for this particular value?
> >
> > 0x50 is the size of struct drm_mode_get_connector. In v3.12-rc7~26^2~2 a
> > u32 padding had been added into the structure, which made its size 0x50.
> > That is, in an older version, its size is 0x4c so in this case if we use
> > ioctl_lookup, the corresponding item in ioctlent0.h will not be found.
> > It's supposed to be { "DRM_IOCTL_MODE_GETCONNECTOR", 0xc05064a7 }, but
> > in the case mentioned above, the ioctl code is 0xc04c64a7.
> > Actually I've discussed a littel bit about this with esyr and I got some
> > sugguestions from him. Details are in [1].
> > [1]
> https://lists.strace.io/pipermail/strace-devel/2019-April/008710.html
>
> Then it makes sense to do this just for 0x4c and add an explanatory
> comment.
>
> > > > +                         } else {
> > > > +                                 return 0;
> > > > +                         }
> > > > +         }
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int
> > > > +drm_set_version(struct tcb *const tcp, long arg)
> > > > +{
> > > > + struct drm_set_version ver;
> > > > +
> > > > + if (entering(tcp)) {
> > > > +         tprints(", ");
> > > > +         if (umove_or_printaddr(tcp, arg, &ver))
> > > > +                 return RVAL_IOCTL_DECODED;
> > > > +         PRINT_FIELD_D("write:{", ver, drm_di_major);
> > > > +         PRINT_FIELD_D(", ", ver, drm_di_minor);
> > > > +         PRINT_FIELD_D(", ", ver, drm_dd_major);
> > > > +         PRINT_FIELD_D(", ", ver, drm_dd_minor);
> > > > +         tprints("}");
> > > > +
> > > > +         return 0;
> > > > + }
> > > > +
> > > > + if (!syserror(tcp) && !umove(tcp, arg, &ver)) {
> > > > +         PRINT_FIELD_D(", read:{", ver, drm_di_major);
> > > > +         PRINT_FIELD_D(", ", ver, drm_di_minor);
> > > > +         PRINT_FIELD_D(", ", ver, drm_dd_major);
> > > > +         PRINT_FIELD_D(", ", ver, drm_dd_minor);
> > > > +         tprints("}");
> > > > + }
> > >
> > > Why does this differ from our traditional way of printing read-write
> > > structures?
> >
> > Any references to the traditional way? I got this from Patrik's original
> > patch and I didn't notice this is against the tradition.
>
> git grep '" => '
>
> > > > +int
> > > > +drm_ioctl(struct tcb *const tcp, const unsigned int code,
> > > > +   const kernel_ulong_t arg)
> > > > +{
> > > > + switch (code) {
> > > > + case DRM_IOCTL_SET_VERSION:
> > > > +         return drm_set_version(tcp, arg);
> > > > + case DRM_IOCTL_GET_MAGIC:
> > > > +         return drm_get_magic(tcp, arg);
> > > > + case DRM_IOCTL_MODE_GETRESOURCES:
> > > > +         return drm_mode_get_resources(tcp, arg);
> > > > + case DRM_IOCTL_MODE_GETCRTC:
> > > > +         return drm_mode_get_crtc(tcp, arg);
> > > > + case DRM_IOCTL_MODE_SETCRTC:
> > > > +         return drm_mode_set_crtc(tcp, arg);
> > > > + case DRM_IOCTL_MODE_CURSOR:
> > > > +         return drm_mode_cursor(tcp, arg);
> > > > + case DRM_IOCTL_MODE_CURSOR2:
> > > > +         return drm_mode_cursor2(tcp, arg);
> > > > + case DRM_IOCTL_MODE_GETGAMMA:
> > > > +         return drm_mode_get_gamma(tcp, arg);
> > > > + case DRM_IOCTL_MODE_SETGAMMA:
> > > > +         return drm_mode_set_gamma(tcp, arg);
> > > > + case DRM_IOCTL_MODE_GETENCODER:
> > > > +         return drm_mode_get_encoder(tcp, arg);
> > > > + case DRM_IOCTL_MODE_GETPROPERTY:
> > > > +         return drm_mode_get_property(tcp, arg);
> > > > + case DRM_IOCTL_MODE_SETPROPERTY:
> > > > +         return drm_mode_set_property(tcp, arg);
> > > > + case DRM_IOCTL_MODE_GETPROPBLOB:
> > > > +         return drm_mode_get_prop_blob(tcp, arg);
> > > > + case DRM_IOCTL_MODE_GETFB:
> > > > +         return drm_mode_get_fb(tcp, arg);
> > > > + case DRM_IOCTL_MODE_ADDFB:
> > > > +         return drm_mode_add_fb(tcp, arg);
> > > > + case DRM_IOCTL_MODE_RMFB:
> > > > +         return drm_mode_rm_fb(tcp, arg);
> > > > + case DRM_IOCTL_MODE_PAGE_FLIP:
> > > > +         return drm_mode_page_flip(tcp, arg);
> > > > + case DRM_IOCTL_MODE_DIRTYFB:
> > > > +         return drm_mode_dirty_fb(tcp, arg);
> > > > + case DRM_IOCTL_MODE_CREATE_DUMB:
> > > > +         return drm_mode_create_dumb(tcp, arg);
> > > > + case DRM_IOCTL_MODE_MAP_DUMB:
> > > > +         return drm_mode_map_dumb(tcp, arg);
> > > > + case DRM_IOCTL_MODE_DESTROY_DUMB:
> > > > +         return drm_mode_destroy_dumb(tcp, arg);
> > > > + case DRM_IOCTL_GEM_CLOSE:
> > > > +         return drm_gem_close(tcp, arg);
> > > > + default: {
> > > > +                 int rc = drm_ioctl_mpers(tcp, code, arg);
> > > > +                 if (rc != RVAL_DECODED)
> > > > +                         return rc;
> > >
> > > Why RVAL_DECODED is filtered here?
> > >
> >
> > I took v4.22~55 "evdev: move mpers-specific parsers to a separate file"
> > as reference. Based on what I've learned about RVAL_* macros, I think
> > in this case RVAL_DECODED means the decoding procedure has to be
> > continued. Because in drm_mpers.c RVAL_DECODED is returned when no cases
> > match the ioctl code.
>
> In evdev.c multi-number fixed-length commands are handled after
> evdev_write_ioctl_mpers invocation, that explains RVAL_DECODED check
> there.  I don't see anything similar in drm.c and the check for
> RVAL_DECODED looks redundant.
>
> > > > +#include DEF_MPERS_TYPE(struct_drm_version)
> > > > +#include DEF_MPERS_TYPE(struct_drm_unique)
> > > > +#include DEF_MPERS_TYPE(union_drm_wait_vblank)
> > > > +#include DEF_MPERS_TYPE(struct_drm_mode_get_connector)
> > > > +#include DEF_MPERS_TYPE(struct_drm_mode_fb_cmd2)
> > >
> > > Interesting.  Are the last two structures really differ between
> > > personalities?
> >
> > As mentioned above, the size of struct drm_mode_get_connector in older
> > versions of headers is 0x4c. In the case of 32 bit aligment, the decoder
> > will try to fetch a struct of 0x4c while in 64 bit, 0x50 will be
> > fetched.
>
> Shouldn't we just handle both old 0x4c and 0x50 sizes regardless of
> personality?
>
> > As for struct drm_mode_fb_cmd2, I see that in drivers/gpu/drm/drm_ioc32.c
> > there is a compat struct drm_mode_fb_cmd232 with __attribute__((packed))
> > flag. Shouldn't it be different between personalities?
>
> Yes, there is a difference indeed.
>
>
> --
> ldv
> --
> Strace-devel mailing list
> Strace-devel at lists.strace.io
> https://lists.strace.io/mailman/listinfo/strace-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20190616/ee3ad475/attachment.html>


More information about the Strace-devel mailing list