[PATCH libdrm 1/4] freedreno: zero is a valid fd number, treat it as such

[PATCH libdrm 1/4] freedreno: zero is a valid fd number, treat it as such

[Emil Velikov]

Abeit quite unlikely to get hit by this bug here, let just fix it.

v2: Correct conditional (do not call ioctl(DRM_IOCTL_PRIME_HANDLE_TO_FD)
when we already have the fd).
v3: Fix kgsl_pipe.c, suggested by Thierry.

Cc: freedreno@lists.freedesktop.org
Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Reviewed-by: Thierry Reding <thierry.reding@gmail.com>
---
 freedreno/freedreno_bo.c   | 6 +++---
 freedreno/kgsl/kgsl_bo.c   | 1 +
 freedreno/kgsl/kgsl_pipe.c | 2 +-
 freedreno/msm/msm_bo.c     | 1 +
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/freedreno/freedreno_bo.c b/freedreno/freedreno_bo.c
index 517a2f8..eec218c 100644
--- a/freedreno/freedreno_bo.c
+++ b/freedreno/freedreno_bo.c
@@ -285,9 +285,9 @@ void fd_bo_del(struct fd_bo *bo)
 	if (!atomic_dec_and_test(&bo->refcnt))
 		return;
 
-	if (bo->fd) {
+	if (bo->fd >= 0) {
 		close(bo->fd);
-		bo->fd = 0;
+		bo->fd = -1;
 	}
 
 	pthread_mutex_lock(&table_lock);
@@ -372,7 +372,7 @@ uint32_t fd_bo_handle(struct fd_bo *bo)
 
 int fd_bo_dmabuf(struct fd_bo *bo)
 {
-	if (!bo->fd) {
+	if (bo->fd < 0) {
 		struct drm_prime_handle req = {
 				.handle = bo->handle,
 				.flags = DRM_CLOEXEC,
diff --git a/freedreno/kgsl/kgsl_bo.c b/freedreno/kgsl/kgsl_bo.c
index 15c3ff5..3407c76 100644
--- a/freedreno/kgsl/kgsl_bo.c
+++ b/freedreno/kgsl/kgsl_bo.c
@@ -168,6 +168,7 @@ drm_private struct fd_bo * kgsl_bo_from_handle(struct fd_device *dev,
 
 	bo = &kgsl_bo->base;
 	bo->funcs = &funcs;
+	bo->fd = -1;
 
 	for (i = 0; i < ARRAY_SIZE(kgsl_bo->list); i++)
 		list_inithead(&kgsl_bo->list[i]);
diff --git a/freedreno/kgsl/kgsl_pipe.c b/freedreno/kgsl/kgsl_pipe.c
index fc76b2b..08c87a6 100644
--- a/freedreno/kgsl/kgsl_pipe.c
+++ b/freedreno/kgsl/kgsl_pipe.c
@@ -101,7 +101,7 @@ static void kgsl_pipe_destroy(struct fd_pipe *pipe)
 	if (kgsl_pipe->drawctxt_id)
 		ioctl(kgsl_pipe->fd, IOCTL_KGSL_DRAWCTXT_DESTROY, &req);
 
-	if (kgsl_pipe->fd)
+	if (kgsl_pipe->fd >= 0)
 		close(kgsl_pipe->fd);
 
 	free(kgsl_pipe);
diff --git a/freedreno/msm/msm_bo.c b/freedreno/msm/msm_bo.c
index fbd82df..3f5b6d0 100644
--- a/freedreno/msm/msm_bo.c
+++ b/freedreno/msm/msm_bo.c
@@ -137,6 +137,7 @@ drm_private struct fd_bo * msm_bo_from_handle(struct fd_device *dev,
 
 	bo = &msm_bo->base;
 	bo->funcs = &funcs;
+	bo->fd = -1;
 
 	for (i = 0; i < ARRAY_SIZE(msm_bo->list); i++)
 		list_inithead(&msm_bo->list[i]);
-- 
2.4.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 2/4] omap: zero is a valid fd number, treat it as such

[Emil Velikov]

Equivalent to the previous patch.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Suggested-by: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 omap/omap_drm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/omap/omap_drm.c b/omap/omap_drm.c
index ff83a93..4a0248d 100644
--- a/omap/omap_drm.c
+++ b/omap/omap_drm.c
@@ -363,7 +363,7 @@ void omap_bo_del(struct omap_bo *bo)
 		munmap(bo->map, bo->size);
 	}
 
-	if (bo->fd) {
+	if (bo->fd >= 0) {
 		close(bo->fd);
 	}
 
@@ -414,7 +414,7 @@ uint32_t omap_bo_handle(struct omap_bo *bo)
  */
 int omap_bo_dmabuf(struct omap_bo *bo)
 {
-	if (!bo->fd) {
+	if (bo->fd < 0) {
 		struct drm_prime_handle req = {
 				.handle = bo->handle,
 				.flags = DRM_CLOEXEC,
-- 
2.4.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 3/4] xf86drm: fix incorrect fd comparison in drmOpenOnce{, WithType}

[Emil Velikov]

Spotted by looking for similar "let's assume fd == 0 is invalid" bugs.

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 xf86drm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xf86drm.c b/xf86drm.c
index 2c17d11..39c6e2d 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2619,7 +2619,7 @@ int drmOpenOnceWithType(const char *BusID, int *newlyopened, int type)
 	}
 
     fd = drmOpenWithType(NULL, BusID, type);
-    if (fd <= 0 || nr_fds == DRM_MAX_FDS)
+    if (fd < 0 || nr_fds == DRM_MAX_FDS)
 	return fd;
    
     connection[nr_fds].BusID = strdup(BusID);
-- 
2.4.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 4/4] Consistently check the fd value

[Emil Velikov]

Follow the approach used through the rest of the project.

Cc: Thierry Reding <thierry.reding@gmail.com>
Suggested-by: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 intel/test_decode.c  | 2 +-
 tests/name_from_fd.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/intel/test_decode.c b/intel/test_decode.c
index bef86bb..b4eddcd 100644
--- a/intel/test_decode.c
+++ b/intel/test_decode.c
@@ -56,7 +56,7 @@ read_file(const char *filename, void **ptr, size_t *size)
 	struct stat st;
 
 	fd = open(filename, O_RDONLY);
-	if (fd == -1)
+	if (fd < 0)
 		errx(1, "couldn't open `%s'", filename);
 
 	ret = fstat(fd, &st);
diff --git a/tests/name_from_fd.c b/tests/name_from_fd.c
index 24af6e6..5264681 100644
--- a/tests/name_from_fd.c
+++ b/tests/name_from_fd.c
@@ -45,7 +45,7 @@ int main(int argc, char **argv)
 	char *v;
 
 	fd = open("/dev/dri/card0", O_RDWR);
-	if (fd == -1)
+	if (fd < 0)
 		return 0;
 
 	v = drmGetDeviceNameFromFd(fd);
-- 
2.4.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 2/4] omap: zero is a valid fd number, treat it as such

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1050 bytes --]

On Tue, Jul 14, 2015 at 03:10:03PM +0100, Emil Velikov wrote:
> Equivalent to the previous patch.
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> ---
>  omap/omap_drm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

> diff --git a/omap/omap_drm.c b/omap/omap_drm.c
> index ff83a93..4a0248d 100644
> --- a/omap/omap_drm.c
> +++ b/omap/omap_drm.c
> @@ -363,7 +363,7 @@ void omap_bo_del(struct omap_bo *bo)
>  		munmap(bo->map, bo->size);
>  	}
>  
> -	if (bo->fd) {
> +	if (bo->fd >= 0) {
>  		close(bo->fd);
>  	}
>  
> @@ -414,7 +414,7 @@ uint32_t omap_bo_handle(struct omap_bo *bo)
>   */
>  int omap_bo_dmabuf(struct omap_bo *bo)
>  {
> -	if (!bo->fd) {
> +	if (bo->fd < 0) {
>  		struct drm_prime_handle req = {
>  				.handle = bo->handle,
>  				.flags = DRM_CLOEXEC,
> -- 
> 2.4.5
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 3/4] xf86drm: fix incorrect fd comparison in drmOpenOnce{,WithType}

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1155 bytes --]

On Tue, Jul 14, 2015 at 03:10:04PM +0100, Emil Velikov wrote:
> Spotted by looking for similar "let's assume fd == 0 is invalid" bugs.
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> ---
>  xf86drm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 2c17d11..39c6e2d 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -2619,7 +2619,7 @@ int drmOpenOnceWithType(const char *BusID, int *newlyopened, int type)
>  	}
>  
>      fd = drmOpenWithType(NULL, BusID, type);
> -    if (fd <= 0 || nr_fds == DRM_MAX_FDS)
> +    if (fd < 0 || nr_fds == DRM_MAX_FDS)

Consider what happens if we have DRM_MAX_FDS file descriptors open and
the call to drmOpenWithType() succeeds. We'll end up returning the file
descriptor as is, but we won't keep track.

I suppose this could have been on purpose, so that the device could be
opened even if the file descriptor couldn't be cached anymore. One
potential problem with that could be that the open-once restriction
would be silently ignored. That may not be desirable.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 4/4] Consistently check the fd value

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1258 bytes --]

On Tue, Jul 14, 2015 at 03:10:05PM +0100, Emil Velikov wrote:
> Follow the approach used through the rest of the project.
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> ---
>  intel/test_decode.c  | 2 +-
>  tests/name_from_fd.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

> diff --git a/intel/test_decode.c b/intel/test_decode.c
> index bef86bb..b4eddcd 100644
> --- a/intel/test_decode.c
> +++ b/intel/test_decode.c
> @@ -56,7 +56,7 @@ read_file(const char *filename, void **ptr, size_t *size)
>  	struct stat st;
>  
>  	fd = open(filename, O_RDONLY);
> -	if (fd == -1)
> +	if (fd < 0)
>  		errx(1, "couldn't open `%s'", filename);
>  
>  	ret = fstat(fd, &st);
> diff --git a/tests/name_from_fd.c b/tests/name_from_fd.c
> index 24af6e6..5264681 100644
> --- a/tests/name_from_fd.c
> +++ b/tests/name_from_fd.c
> @@ -45,7 +45,7 @@ int main(int argc, char **argv)
>  	char *v;
>  
>  	fd = open("/dev/dri/card0", O_RDWR);
> -	if (fd == -1)
> +	if (fd < 0)
>  		return 0;
>  
>  	v = drmGetDeviceNameFromFd(fd);
> -- 
> 2.4.5
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 3/4] xf86drm: fix incorrect fd comparison in drmOpenOnce{, WithType}

[Emil Velikov]

On 15 July 2015 at 12:47, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Jul 14, 2015 at 03:10:04PM +0100, Emil Velikov wrote:
>> Spotted by looking for similar "let's assume fd == 0 is invalid" bugs.
>>
>> Cc: Thierry Reding <thierry.reding@gmail.com>
>> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>> ---
>>  xf86drm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xf86drm.c b/xf86drm.c
>> index 2c17d11..39c6e2d 100644
>> --- a/xf86drm.c
>> +++ b/xf86drm.c
>> @@ -2619,7 +2619,7 @@ int drmOpenOnceWithType(const char *BusID, int *newlyopened, int type)
>>       }
>>
>>      fd = drmOpenWithType(NULL, BusID, type);
>> -    if (fd <= 0 || nr_fds == DRM_MAX_FDS)
>> +    if (fd < 0 || nr_fds == DRM_MAX_FDS)
>
> Consider what happens if we have DRM_MAX_FDS file descriptors open and
> the call to drmOpenWithType() succeeds. We'll end up returning the file
> descriptor as is, but we won't keep track.
>
> I suppose this could have been on purpose, so that the device could be
> opened even if the file descriptor couldn't be cached anymore. One
> potential problem with that could be that the open-once restriction
> would be silently ignored. That may not be desirable.
>
Thanks for reviewing !

Yes I have considered the issue. It's slightly different bug (fixed by
using dynamic allocation?) than what this patch aims at. This fix came
along for consistency sake rather than me caring about this legacy
API.

Can you please follow up if you're interested ?

Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 3/4] xf86drm: fix incorrect fd comparison in drmOpenOnce{,WithType}

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2052 bytes --]

On Wed, Jul 15, 2015 at 01:37:22PM +0100, Emil Velikov wrote:
> On 15 July 2015 at 12:47, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Tue, Jul 14, 2015 at 03:10:04PM +0100, Emil Velikov wrote:
> >> Spotted by looking for similar "let's assume fd == 0 is invalid" bugs.
> >>
> >> Cc: Thierry Reding <thierry.reding@gmail.com>
> >> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> >> ---
> >>  xf86drm.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/xf86drm.c b/xf86drm.c
> >> index 2c17d11..39c6e2d 100644
> >> --- a/xf86drm.c
> >> +++ b/xf86drm.c
> >> @@ -2619,7 +2619,7 @@ int drmOpenOnceWithType(const char *BusID, int *newlyopened, int type)
> >>       }
> >>
> >>      fd = drmOpenWithType(NULL, BusID, type);
> >> -    if (fd <= 0 || nr_fds == DRM_MAX_FDS)
> >> +    if (fd < 0 || nr_fds == DRM_MAX_FDS)
> >
> > Consider what happens if we have DRM_MAX_FDS file descriptors open and
> > the call to drmOpenWithType() succeeds. We'll end up returning the file
> > descriptor as is, but we won't keep track.
> >
> > I suppose this could have been on purpose, so that the device could be
> > opened even if the file descriptor couldn't be cached anymore. One
> > potential problem with that could be that the open-once restriction
> > would be silently ignored. That may not be desirable.
> >
> Thanks for reviewing !
> 
> Yes I have considered the issue. It's slightly different bug (fixed by
> using dynamic allocation?) than what this patch aims at. This fix came
> along for consistency sake rather than me caring about this legacy
> API.

Right, I had meant to say:

Reviewed-by: Thierry Reding <treding@nvidia.com>

irrespective of the above, given that they are two orthogonal issues.

> Can you please follow up if you're interested ?

I don't think I have any interest in this API, just occurred to me that
it wasn't well documented and the behaviour seemed at odds with what
documentation there is (i.e. the function name).

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 3/4] xf86drm: fix incorrect fd comparison in drmOpenOnce{, WithType}

[Emil Velikov]

On 15 July 2015 at 13:47, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Jul 15, 2015 at 01:37:22PM +0100, Emil Velikov wrote:
>> On 15 July 2015 at 12:47, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > On Tue, Jul 14, 2015 at 03:10:04PM +0100, Emil Velikov wrote:
>> >> Spotted by looking for similar "let's assume fd == 0 is invalid" bugs.
>> >>
>> >> Cc: Thierry Reding <thierry.reding@gmail.com>
>> >> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>> >> ---
>> >>  xf86drm.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/xf86drm.c b/xf86drm.c
>> >> index 2c17d11..39c6e2d 100644
>> >> --- a/xf86drm.c
>> >> +++ b/xf86drm.c
>> >> @@ -2619,7 +2619,7 @@ int drmOpenOnceWithType(const char *BusID, int *newlyopened, int type)
>> >>       }
>> >>
>> >>      fd = drmOpenWithType(NULL, BusID, type);
>> >> -    if (fd <= 0 || nr_fds == DRM_MAX_FDS)
>> >> +    if (fd < 0 || nr_fds == DRM_MAX_FDS)
>> >
>> > Consider what happens if we have DRM_MAX_FDS file descriptors open and
>> > the call to drmOpenWithType() succeeds. We'll end up returning the file
>> > descriptor as is, but we won't keep track.
>> >
>> > I suppose this could have been on purpose, so that the device could be
>> > opened even if the file descriptor couldn't be cached anymore. One
>> > potential problem with that could be that the open-once restriction
>> > would be silently ignored. That may not be desirable.
>> >
>> Thanks for reviewing !
>>
>> Yes I have considered the issue. It's slightly different bug (fixed by
>> using dynamic allocation?) than what this patch aims at. This fix came
>> along for consistency sake rather than me caring about this legacy
>> API.
>
> Right, I had meant to say:
>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
>
> irrespective of the above, given that they are two orthogonal issues.
>
Ack. Greatly appreciated.

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel