[RFC PATCH v1 2/3] drm: implement decoding of DRM ioctls
Dmitry V. Levin
ldv at altlinux.org
Thu Jun 13 14:38:14 UTC 2019
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.
> > > +
> > > +#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
More information about the Strace-devel
mailing list