Re: [PATCH 1/4] drm/vkms: Add functions to map GEM backing storage

Quoting Haneen Mohammed (2018-07-09 16:44:26)

+struct page **get_pages(struct vkms_gem_object *vkms_obj) +{

•   struct drm_gem_object *gem_obj = &vkms_obj->gem;
  

•   struct page **pages = vkms_obj->pages;
  

•   if (!pages) {
  

•           mutex_lock(&vkms_obj->pages_lock);
  

•           pages = drm_gem_get_pages(gem_obj);
  

•           if (IS_ERR(pages)) {
  

•                   mutex_unlock(&vkms_obj->pages_lock);
  

•                   return pages;
  

•           }
  

•           vkms_obj->pages = pages;
  

•           mutex_unlock(&vkms_obj->pages_lock);
  

You have a race here with two callers setting pages. Trivially you fix it by checking if (!pages) again inside the lock, but the lock is superfluous in this case: if (!vkms_obj->pages) { srtuct pages **pages;

pages = drm_gem_get_pages(gem_obj); if (IS_ERR(pages)) return pages; if (cmpxchg(&vkms_obj->pages, NULL, pages)) put_pages(pages);

/* barrier() is implied */ }

return vkms_obj->pages; -Chris