[RFC PATCH v1 2/3] drm: implement decoding of DRM ioctls
Zhibin Li
haoyouab at gmail.com
Fri Jun 14 16:22:33 UTC 2019
On Thu, Jun 13, 2019 at 05:38:14PM +0300, Dmitry V. Levin 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.
>
OK I will try ifdef the whole bulk. And now the ifdef in ioctl.c for drm
parsers should not be removed.
> > > > +
> > > > +#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.
>
Will do.
> > > > + } 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 '" => '
>
Will take a look and change this printing function to traditional
fashion later.
> > > > +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.
>
Now I see.
> > > > +#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?
>
It's a little bit tricky and I think I didn't make my point clear. I will
try to elaborate again.
In the case of newer kernel headers, the size of this structure is 0x50
and there is no difference between personalities when we use
umove_or_printaddr to fetch such a structure. Because on both 32-bit and
64-bit alignment it's 0x50.
But in the case of old kernel headers, its size is 0x4c. If I'm not
understanding in the wrong way, on 32-bit machine umove_or_printaddr
will try to fetch a structure of 0x4c while on 64-bit machine
umove_or_printaddr will try to fetch a 80-byte structure even if its
actual size is 76 bytes because of 8-byte alignment. Hence in such a
case it differs between personalities.
So yes both sizes should be handled and only the case of old headers
should be handled differently between personalities.
> > 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
More information about the Strace-devel
mailing list