linux-kernel - Re: [PATCH 3/6] ksmbd: replace rwlock with rcu for concurrenct access on conn list

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]

Message-ID: <Y9h/ZbLPQlbKiRzZ@google.com>
Date:   Tue, 31 Jan 2023 11:39:33 +0900
From:   Sergey Senozhatsky <senozhatsky@...omium.org>
To:     Dawei Li <set_pte_at@...look.com>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        linkinjeon@...nel.org, sfrench@...ba.org, tom@...pey.com,
        hyc.lee@...il.com, linux-cifs@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] ksmbd: replace rwlock with rcu for concurrenct
 access on conn list

Hi,

On (23/01/30 22:16), Dawei Li wrote:
> Hi Sergey,
> 
> Thanks for reviewing,
> 
> On Mon, Jan 30, 2023 at 01:15:35PM +0900, Sergey Senozhatsky wrote:
> > On (23/01/15 18:32), Dawei Li wrote:
> > > 
> > >  void ksmbd_conn_free(struct ksmbd_conn *conn)
> > >  {
> > > -	write_lock(&conn_list_lock);
> > > -	list_del(&conn->conns_list);
> > > -	write_unlock(&conn_list_lock);
> > > +	spin_lock(&conn_list_lock);
> > > +	list_del_rcu(&conn->conns_list);
> > > +	spin_unlock(&conn_list_lock);
>         synchronize_rcu(); 
> > >  
> > >  	xa_destroy(&conn->sessions);
> > >  	kvfree(conn->request_buf);
> > 
> > From a quick look this does not seem like a correct RCU usage. E.g.
> > where do you wait for grace periods and synchronize readers/writers?
> 
> Nice catch, I totally mess it up. Thanks!
> 
> At first glance, I assume synchronize_rcu() will do the job if sleeping
> is OK?

Yes, synchronize_rcu() will sleep (schedule()) and wait for grace
period to expire (and synchronize will all the RCU readers). RCU
is good for cases when writes are seldom, which may not be the case
with ksmb.

I really want to see benhcmarks, why do we want to remove the RW-lock.