Re: [bug report] drm/xe: Enforce correct user fence signaling order using

[Dan Carpenter <dan.carpenter@linaro.org>]

On Tue, Nov 18, 2025 at 09:05:15AM -0800, Matthew Brost wrote:
> On Tue, Nov 18, 2025 at 06:14:54PM +0300, Dan Carpenter wrote:
> > Hello Matthew Brost,
> > 
> > Commit adda4e855ab6 ("drm/xe: Enforce correct user fence signaling
> > order using") from Oct 31, 2025 (linux-next), leads to the following
> > Smatch static checker warning:
> > 
> > 	drivers/gpu/drm/xe/xe_oa.c:1867 xe_oa_stream_open_ioctl_locked()
> > 	error: double free of 'param->syncs' (line 1863)
> > 
> > drivers/gpu/drm/xe/xe_oa.c
> >     1831 static int xe_oa_stream_open_ioctl_locked(struct xe_oa *oa,
> >     1832                                           struct xe_oa_open_param *param)
> >     1833 {
> >     1834         struct xe_oa_stream *stream;
> >     1835         struct drm_syncobj *ufence_syncobj;
> >     1836         int stream_fd;
> >     1837         int ret;
> >     1838 
> >     1839         /* We currently only allow exclusive access */
> >     1840         if (param->oa_unit->exclusive_stream) {
> >     1841                 drm_dbg(&oa->xe->drm, "OA unit already in use\n");
> >     1842                 ret = -EBUSY;
> >     1843                 goto exit;
> >     1844         }
> >     1845 
> >     1846         ret = drm_syncobj_create(&ufence_syncobj, DRM_SYNCOBJ_CREATE_SIGNALED,
> >     1847                                  NULL);
> >     1848         if (ret)
> >     1849                 goto exit;
> >     1850 
> >     1851         stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> >     1852         if (!stream) {
> >     1853                 ret = -ENOMEM;
> >     1854                 goto err_syncobj;
> >     1855         }
> >     1856         stream->ufence_syncobj = ufence_syncobj;
> >     1857         stream->oa = oa;
> >     1858 
> >     1859         ret = xe_oa_parse_syncs(oa, stream, param);
> >     1860         if (ret)
> >     1861                 goto err_free;
> >     1862 
> >     1863         ret = xe_oa_stream_init(stream, param);
> >     1864         if (ret) {
> >     1865                 while (param->num_syncs--)
> >     1866                         xe_sync_entry_cleanup(&param->syncs[param->num_syncs]);
> > --> 1867                 kfree(param->syncs);
> >                          ^^^^^^^^^^^^^^^^^^^^
> > 
> > xe_oa_stream_init() already frees param->syncs when it calls
> > xe_oa_emit_oa_config().
> > 
> 
> Admittedly this coded poorly but I think this a false positive.
> param->syncs is only freed when xe_oa_stream_init returns success.
> 
> That said this should probably be refactored a bit for clarity.
> 

Sorry for this.  It's a bug in Smatch, yes.  I'm testing a fix for this.

Thanks for looking into it.

regards,
dan carpenter


> Matt
> 
> >     1868                 goto err_free;
> >     1869         }
> >     1870 
> >     1871         if (!param->disabled) {
> >     1872                 ret = xe_oa_enable_locked(stream);
> >     1873                 if (ret)
> >     1874                         goto err_destroy;
> >     1875         }
> >     1876 
> >     1877         stream_fd = anon_inode_getfd("[xe_oa]", &xe_oa_fops, stream, 0);
> >     1878         if (stream_fd < 0) {
> >     1879                 ret = stream_fd;
> >     1880                 goto err_disable;
> >     1881         }
> >     1882 
> >     1883         /* Hold a reference on the drm device till stream_fd is released */
> >     1884         drm_dev_get(&stream->oa->xe->drm);
> >     1885 
> >     1886         return stream_fd;
> >     1887 err_disable:
> >     1888         if (!param->disabled)
> >     1889                 xe_oa_disable_locked(stream);
> >     1890 err_destroy:
> >     1891         xe_oa_stream_destroy(stream);
> >     1892 err_free:
> >     1893         kfree(stream);
> >     1894 err_syncobj:
> >     1895         drm_syncobj_put(ufence_syncobj);
> >     1896 exit:
> >     1897         return ret;
> >     1898 }
> > 
> > regards,
> > dan carpenter