linux-kernel - RE: [RESEND PATCH v2] kernfs: fix dentry unexpected skip

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: <33710E6CAA200E4583255F4FB666C4E21B65E08B@G01JPEXMBYT03>
Date:   Mon, 4 Jun 2018 09:46:17 +0000
From:   "Hatayama, Daisuke" <d.hatayama@...fujitsu.com>
To:     "'tj@...nel.org'" <tj@...nel.org>
CC:     "'gregkh@...uxfoundation.org'" <gregkh@...uxfoundation.org>,
        "Okajima, Toshiyuki" <toshi.okajima@...fujitsu.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "'ebiederm@...stanetworks.com'" <ebiederm@...stanetworks.com>
Subject: RE: [RESEND PATCH v2] kernfs: fix dentry unexpected skip



> -----Original Message-----
> From: Tejun Heo [mailto:htejun@...il.com] On Behalf Of 'tj@...nel.org'
> Sent: Saturday, June 2, 2018 2:07 AM
> To: Hatayama, Daisuke <d.hatayama@...fujitsu.com>
> Cc: 'gregkh@...uxfoundation.org' <gregkh@...uxfoundation.org>; Okajima,
> Toshiyuki <toshi.okajima@...fujitsu.com>;
> linux-kernel@...r.kernel.org; 'ebiederm@...stanetworks.com'
> <ebiederm@...stanetworks.com>
> Subject: Re: [RESEND PATCH v2] kernfs: fix dentry unexpected skip
> 
> Hello,
> 
> On Fri, Jun 01, 2018 at 09:25:32AM +0000, Hatayama, Daisuke wrote:
> > kernfs_dir_pos() checks if a kernfs_node object given as one of its
> > arguments is still active and if so returns it, or returns a
> > kernfs_node object that is most equal (possibly smaller and larger) to
> > the given object.
> 
> Sometimes they're duplicate operations tho, which is exactly the bug
> the posted patch is trying to fix.  What I'm suggesting is instead of
> leaving both instances and skipping one conditionally, put them in one
> place and trigger only when necessary.  The sequence of operations
> would be exactly the same.  The only difference is how the code is
> organized.
> 

I see and I think Eric's patch is written as you suggest very well.

> > kernfs_dir_next_pos() returns a kernfs_node object that is next to the
> > object given by kernfs_dir_pos().
> >
> > Two functions does different things and both need to skip inactive
> > nodes. I don't think it natural to remove the skip only from
> > kernfs_dir_pos().
> >
> > OTOH, throughout getdents(), there is no case that the kernfs_node
> > object given to kernfs_dir_pos() is used afterwards in the
> > processing. So, is it enough to provide kernfs_dir_next_pos() only?
> > Then, the skip code is now not duplicated.
> >
> > The patch below is my thought. How do you think?
> >
> > But note that this patch has some bug so that system boot get hang
> > without detecting root filesystem disk :) I'm debugging this now.
> 
> I haven't looked into the code that closely but given that we had
> cases where both skippings were fine and not, the condition is likely
> gonna be a bit tricker?

I agree to this version looks a bit tricker. But I think once the skipping
code is separated as Eric's patch, it would be resolved naturally.

> 
> Thanks.
> 
> --
> tejun
>