linux-kernel - Re: [patch] f2fs: use _safe() version of list_for

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]

Message-ID: <alpine.DEB.2.02.1301211135300.1930@hadrien>
Date:	Mon, 21 Jan 2013 11:37:13 +0100 (CET)
From:	Julia Lawall <julia.lawall@...6.fr>
To:	Namjae Jeon <linkinjeon@...il.com>
cc:	jaegeuk.kim@...sung.com,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	linux-f2fs-devel@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [patch] f2fs: use _safe() version of list_for_each

On Mon, 21 Jan 2013, Namjae Jeon wrote:

> 2013/1/21, Jaegeuk Kim <jaegeuk.kim@...sung.com>:
> > 2013-01-21 (월), 00:32 -0800, Dmitry Torokhov:
> >> On Sun, Jan 20, 2013 at 06:02:58PM +0300, Dan Carpenter wrote:
> >> > This is calling list_del() inside a loop which is a problem when we try
> >> > move to the next item on the list.  I've converted it to use the _safe
> >> > version.  And also, as a cleanup, I've converted it to use
> >> > list_for_each_entry instead of list_for_each.
> >> >
> >> > Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
> >> > ---
> >> > Static analysis stuff.  Untested.  Please review carefully.
> >>
> >> Makes sense to me.
> >>
> >> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> >>
> >
> > No doubt, applied.
> > Thanks,
> I agree in cases – where we will have chances of parallel access and
> modification to the linked we should use list_for_each_entry_safe()
> But my point was related with this code change case in the patch.
> We will have path like this:
> f2fs_fill_super->recover_fsync_data->destroy_fsync_dnodes()
> From this calling path – there can only be a single caller at any
> given time. So, we will not have the case of having parallel access to
> the list which is local to recover_fsync_data() and destroyed on exit
> from this function.
> From the real issue point of view – this does not looks convincing to
> me why expensive _safe fucntion should used.

The need for the safe version has nothing to do with parallelism.  It has
to do with whether the loop deletes some of the elements in the list.
The non-safe list mechanism uses the current element to get to the next
element.  If the current element has been deleted, it cannot get to the
next one.

julia