| Message ID | 1435755168-16207-3-git-send-email-patrik.jakobsson@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 03, 2015 at 03:33:31AM +0300, Dmitry V. Levin wrote: > 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: Yes, that's more robust. I was afraid it would be too intrusive. > > * 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. > > [...] I'll get that sorted out. > > +#include "defs.h" > > + > > +#include <drm.h> > > +#include <linux/limits.h> > > Please include <sys/param.h> instead of <linux/limits.h>. Yup > > +#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)); > } > That's nicer > > + > > +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. Ok > > + 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 consistently call it "code" to > distinguish from its argument. > > [...] > > --- a/ioctl.c > > +++ b/ioctl.c > > @@ -182,7 +182,7 @@ hiddev_decode_number(unsigned int arg) > > } > > > > int > > -ioctl_decode_command_number(unsigned int arg) > > +ioctl_decode_command_number(struct tcb *tcp, unsigned int arg) > > I've already changed ioctl_decode_command_number's signature: > struct tcb * is already there and "arg" is now called "code". > > > -- > ldv
diff --git a/defs.h b/defs.h index c02f810..5b0670e 100644 --- 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; + #if defined(LINUX_MIPSN32) || defined(X32) long long ext_arg[MAX_ARGS]; long long u_lrval; /* long long return value */
This field can be used by the decode functions of a syscall. The main usecase for now is to allow storing the driver name for drm devices across the lifetime of a trace control block. This allows us to do the driver identification once and reuse the information in all the decode stages of the ioctl. * defs.h: Add private data field in struct tcb Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> --- defs.h | 7 +++++++ 1 file changed, 7 insertions(+)