Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] drm/syncobj: mixing __free() and goto cleanup in drm_syncobj_eventfd_ioctl() --
commit 235e12bb4f2858e1c0f1b573b50d4958dd229203 Author: Bence Csokas <[email protected]> drm/syncobj: Use scope-based cleanup for eventfd_ctx This commit converts ev_fd_ctx in drm_syncobj_eventfd_ioctl() to use __free(eventfd) and no_free_ptr(), removing the err_kzalloc error label. > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index f1d4da1685457..40bedd30b2fc0 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -1429,9 +1429,9 @@ int > drm_syncobj_eventfd_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_private) > { > + struct eventfd_ctx *ev_fd_ctx __free(eventfd) = NULL; > struct drm_syncobj_eventfd *args = data; > struct drm_syncobj *syncobj; > - struct eventfd_ctx *ev_fd_ctx; > struct syncobj_eventfd_entry *entry; > int ret; > > @@ -1457,10 +1457,10 @@ drm_syncobj_eventfd_ioctl(struct drm_device *dev, > void *data, > entry = kzalloc_obj(*entry); > if (!entry) { > ret = -ENOMEM; > - goto err_kzalloc; > + goto err_fdget; > } > entry->syncobj = syncobj; > - entry->ev_fd_ctx = ev_fd_ctx; > + entry->ev_fd_ctx = no_free_ptr(ev_fd_ctx); > entry->point = args->point; > entry->flags = args->flags; > > @@ -1469,8 +1469,6 @@ drm_syncobj_eventfd_ioctl(struct drm_device *dev, void > *data, > > return 0; > > -err_kzalloc: > - eventfd_ctx_put(ev_fd_ctx); > err_fdget: > drm_syncobj_put(syncobj); > return ret; [Severity: Low] In drivers/gpu/drm/drm_syncobj.c, drm_syncobj_eventfd_ioctl() declares ev_fd_ctx with __free(eventfd) = NULL at the top of the function while retaining goto err_fdget to clean up syncobj. When kzalloc_obj(*entry) fails, jumping to err_fdget runs drm_syncobj_put(syncobj) before scope exit calls eventfd_ctx_put(ev_fd_ctx), which reverses the teardown order relative to acquisition: syncobj = drm_syncobj_find(file_private, args->handle); if (!syncobj) return -ENOENT; ev_fd_ctx = eventfd_ctx_fdget(args->fd); if (IS_ERR(ev_fd_ctx)) { ret = PTR_ERR(ev_fd_ctx); goto err_fdget; } entry = kzalloc_obj(*entry); if (!entry) { ret = -ENOMEM; goto err_fdget; } The documentation in include/linux/cleanup.h notes that __free() variables should be defined and initialized in one statement rather than grouped at the top with = NULL, and states that goto cleanup and scope-based cleanup helpers should not be mixed in the same function: * Given that the "__free(...) = NULL" pattern for variables defined at * the top of the function poses this potential interdependency problem * the recommendation is to always define and assign variables in one * statement and not group variable definitions at the top of the * function when __free() is used. * * Lastly, given that the benefit of cleanup helpers is removal of * "goto", and that the "goto" statement can jump between scopes, the * expectation is that usage of "goto" and cleanup helpers is never * mixed in the same function. I.e. for a given routine, convert all * resources that need a "goto" cleanup to scope-based cleanup, or * convert none of them. Should drm_syncobj_eventfd_ioctl() either convert syncobj to scope-based cleanup as well (or restructure the acquisition order so goto err_fdget is no longer needed), and define ev_fd_ctx at the point where eventfd_ctx_fdget() is called? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
