linux-kernel - Re: [PATCH 1/2] staging: zram: minimize `slot_free

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Mon, 9 Sep 2013 16:21:24 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devel@...verdev.osuosl.org, Minchan Kim <minchan@...nel.org>,
	Jerome Marchand <jmarchan@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)

On Mon, Sep 09, 2013 at 03:49:42PM +0300, Sergey Senozhatsky wrote:
> > > Calling handle_pending_slot_free() for every RW operation may
> > > cause unneccessary slot_free_lock locking, because most likely
> > > process will see NULL slot_free_rq. handle_pending_slot_free()
> > > only when current detects that slot_free_rq is not NULL.
> > > 
> > > v2: protect handle_pending_slot_free() with zram rw_lock.
> > > 
> > 
> > zram->slot_free_lock protects zram->slot_free_rq but shouldn't the zram
> > rw_lock be wrapped around the whole operation like the original code
> > does?  I don't know the zram code, but the original looks like it makes
> > sense but in this one it looks like the locks are duplicative.
> > 
> > Is the down_read() in the original code be changed to down_write()?
> >
> 
> I'm not touching locking around existing READ/WRITE commands.
> 

Your patch does change the locking because now instead of taking the
zram lock once it takes it and then drops it and then retakes it.  This
looks potentially racy to me but I don't know the code so I will defer
to any zram maintainer.

1) You haven't given us any performance numbers so it's not clear if the
   locking is even a problem.

2) The v2 patch introduces an obvious deadlock in zram_slot_free()
   because now we take the rw_lock twice.  Fix your testing to catch
   this kind of bug next time.

3) Explain why it is safe to test zram->slot_free_rq when we are not
   holding the lock.  I think it is unsafe.  I don't want to even think
   about it without the numbers.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/