linux-kernel - Re: [RFC] Don'd hold work_sem while calling worker functions

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] [day] [month] [year] [list]

Date:	Wed, 17 Sep 2014 11:42:35 +0300
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	Richard Weinberger <richard@....at>
Cc:	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC] Don'd hold work_sem while calling worker functions

On Tue, 2014-09-16 at 15:26 +0200, Richard Weinberger wrote:
> I fail to see why we need work_sem while wrk->func() is executed.
> Artem, do you have an idea?
> 
> Having the wear_leveling_worker() called without work_sem held
> would simplify the fastmap code too. I'm currently reworking some
> of it's code and I'm in locking hell. 8-)

Well, the best way for getting clue about the meaning of an R/W
semaphore with an unlimited amount of read-takers I found for myself is
to focus on the write-takers. Read-takers are non-interesting, because
they can race freely.

So let's check write-takers.

There are 2 of them - one in your code, one in the one I wrote many
years ago.

"Mine" is in 'ubi_wl_flush()':

        down_write(&ubi->work_sem);
        up_write(&ubi->work_sem);

And the only reason it is there is to make sure that flush() really
flushes the queue, and when 'ubi_wl_flush()' returns, you may be sure
that all the in-flight works were finished.

There are other ways to achieve this, but I probably found using the R/W
semaphore to be the easiest. Indeed, just make all the works have it in
read mode, and when you have to wait for all the in-flight works to
complete, you take it in write mode - easy.

IOW, this is a bit of an unusual use of R/W semaphores.

HTH.

P.S. Generally, if you have a trouble with a lock, start with checking
the place where it is defined, I tried to document locks there briefly.
And there may be pices of useful comments elsewhere. This should be true
for both UBI and UBIFS. So just a general hint.

For 'work_sem' you'd need to check ubi.h. But unfortunately, the comment
there is not helpful, and even has a typo which makes it confusing.

While on it, would you refine the comment and say something like:

work_sem: used to wait for all the scheduled works to finish and prevent
new works from being submitted

-- 
Best Regards,
Artem Bityutskiy

--
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/