linux-kernel - Re: [Spice-devel] [PATCH] Do not loop on ERESTARTSYS using interruptible waits

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 26 May 2015 10:20:30 +0200
From:	Christophe Fergeau <cfergeau@...hat.com>
To:	Frediano Ziglio <fziglio@...hat.com>
Cc:	spice-devel@...ts.freedesktop.org, David Airlie <airlied@...ux.ie>,
	dri-devel@...ts.freedesktop.org, Dave Airlie <airlied@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [Spice-devel] [PATCH] Do not loop on ERESTARTSYS using
 interruptible waits

Hey,

On Fri, May 22, 2015 at 09:58:10AM -0400, Frediano Ziglio wrote:
> > >  }
> > >  
> > >  int qxl_io_update_area(struct qxl_device *qdev, struct qxl_bo *surf,
> > > -			const struct qxl_rect *area)
> > > +			const struct qxl_rect *area, bool intr)
> > >  {
> > >  	int surface_id;
> > >  	uint32_t surface_width, surface_height;
> > > @@ -350,7 +347,7 @@ int qxl_io_update_area(struct qxl_device *qdev, struct
> > > qxl_bo *surf,
> > >  	mutex_lock(&qdev->update_area_mutex);
> > >  	qdev->ram_header->update_area = *area;
> > >  	qdev->ram_header->update_surface = surface_id;
> > > -	ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, true);
> > > +	ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, intr);
> > >  	mutex_unlock(&qdev->update_area_mutex);
> > >  	return ret;
> > >  }
> > > @@ -588,10 +585,7 @@ int qxl_update_surface(struct qxl_device *qdev, struct
> > > qxl_bo *surf)
> > >  	rect.right = surf->surf.width;
> > >  	rect.top = 0;
> > >  	rect.bottom = surf->surf.height;
> > > -retry:
> > > -	ret = qxl_io_update_area(qdev, surf, &rect);
> > > -	if (ret == -ERESTARTSYS)
> > > -		goto retry;
> > > +	ret = qxl_io_update_area(qdev, surf, &rect, false);
> > 
> > My understanding is that the fix is this hunk? If so, this could be made
> > more obvious with an intermediate commit adding the 'bool intr' arg to
> > qxl_io_update_area and only calling it with 'true' in the appropriate
> > places.
> > This code path is only triggered from qxl_surface_evict() which I assume
> > is not necessarily easily interruptible, so this change makes sense to
> > me. However it would be much better to get a review from Dave Airlie ;)
> > 
> > Christophe
> > 
> 
> Are you asking if just removing the loop would fix the issue?
> So you are proposing a first patch that add the argument always
> passing true and another that change some calls to false? It make
> sense but still the loop should be removed.

Sorry, I was not clear, removing the loop is fine, I was not suggesting
to keep it.

Christophe

Content of type "application/pgp-signature" skipped