linux-kernel - Re: [PATCH 2/5] cifsd: add server-side procedures for SMB3

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: <20210322131244.GM1719932@casper.infradead.org>
Date:   Mon, 22 Mar 2021 13:12:44 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Sergey Senozhatsky <senozhatsky@...omium.org>
Cc:     Namjae Jeon <namjae.jeon@...sung.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-cifs@...r.kernel.org,
        linux-cifsd-devel@...ts.sourceforge.net, smfrench@...il.com,
        hyc.lee@...il.com, viro@...iv.linux.org.uk, hch@....de,
        hch@...radead.org, ronniesahlberg@...il.com,
        aurelien.aptel@...il.com, aaptel@...e.com, sandeen@...deen.net,
        dan.carpenter@...cle.com, colin.king@...onical.com,
        rdunlap@...radead.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steve French <stfrench@...rosoft.com>
Subject: Re: [PATCH 2/5] cifsd: add server-side procedures for SMB3

On Mon, Mar 22, 2021 at 07:27:03PM +0900, Sergey Senozhatsky wrote:
> On (21/03/22 08:34), Matthew Wilcox wrote:
> > > +++ b/fs/cifsd/mgmt/ksmbd_ida.c
> > > @@ -0,0 +1,69 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + *   Copyright (C) 2018 Samsung Electronics Co., Ltd.
> > > + */
> > > +
> > > +#include "ksmbd_ida.h"
> > > +
> > > +struct ksmbd_ida *ksmbd_ida_alloc(void)
> > > +{
> > > +	struct ksmbd_ida *ida;
> > > +
> > > +	ida = kmalloc(sizeof(struct ksmbd_ida), GFP_KERNEL);
> > > +	if (!ida)
> > > +		return NULL;
> > > +
> > > +	ida_init(&ida->map);
> > > +	return ida;
> > > +}
> >
> > ... why?  Everywhere that you call ksmbd_ida_alloc(), you would
> > be better off just embedding the struct ida into the struct that
> > currently has a pointer to it.  Or declaring it statically.  Then
> > you can even initialise it statically using DEFINE_IDA() and
> > eliminate the initialiser functions.
> 
> IIRC this ida is per SMB session, so it probably cannot be static.

Depends which IDA you're talking about.

+struct ksmbd_conn *ksmbd_conn_alloc(void)
+	conn->async_ida = ksmbd_ida_alloc();
Embed into 'conn'.

+static struct ksmbd_ida *ida;
+int ksmbd_ipc_init(void)
+	ida = ksmbd_ida_alloc();
Should be static.

> And Windows, IIRC, doesn't like "just any IDs". Some versions of Windows
> would fail the session login if server would return the first id == 0,
> instead of 1. Or vice versa. I don't remember all the details, the last
> time I looked into this was in 2019.

Sure, you can keep that logic.

> > ... walk the linked list looking for an ID match.  You'd be much better
> > off using an allocating XArray:
> > https://www.kernel.org/doc/html/latest/core-api/xarray.html
> 
> I think cifsd code predates XArray ;)

Sure, but you could have used an IDR ;-)

> > Then you could lookup tree connections in O(log(n)) time instead of
> > O(n) time.
> 
> Agreed. Not sure I remember why the code does list traversal here.