linux-kernel - Re: [PATCH v4 06/17] virt: acrn: Introduce VM management interfaces

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, 28 Sep 2020 07:25:16 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Shuo A Liu <shuo.a.liu@...el.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        "H . Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Yu Wang <yu1.wang@...el.com>,
        Reinette Chatre <reinette.chatre@...el.com>,
        Zhi Wang <zhi.a.wang@...el.com>,
        Zhenyu Wang <zhenyuw@...ux.intel.com>
Subject: Re: [PATCH v4 06/17] virt: acrn: Introduce VM management interfaces

On Mon, Sep 28, 2020 at 11:50:30AM +0800, Shuo A Liu wrote:
> > > +	write_lock_bh(&acrn_vm_list_lock);
> > > +	list_add(&vm->list, &acrn_vm_list);
> > > +	write_unlock_bh(&acrn_vm_list_lock);
> > 
> > Why are the _bh() variants being used here?
> > 
> > You are only accessing this list from userspace context in this patch.
> > 
> > Heck, you aren't even reading from the list, only writing to it...
> 
> acrn_vm_list is read in a tasklet which dispatch I/O requests and is wrote
> in VM creation ioctl. Use the rwlock mechanism to protect it.
> The reading operation is introduced in the following patches of this
> series. So i keep the lock type at the moment of introduction.

Ok, but think about someone trying to review this code.  Does this lock
actually make sense here?  No, it does not.  How am I supposed to know
to look at future patches to determine that it changes location and
usage to require this?

That's just not fair, would you want to review something like this?

And a HUGE meta-comment, again, why am I the only one reviewing this
stuff?  Why do you have a ton of Intel people on the Cc: yet it is, once
again, my job to do this?

If you all are wanting to burn me out, you are doing a good job...

greg k-h