Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190829050237.GA5161@jagdpanzerIV>
In-Reply-To: <20190829050237.GA5161@jagdpanzerIV>
From:   Pavel Shilovsky <piastryyy@gmail.com>
Date:   Thu, 19 Sep 2019 17:11:54 -0700
Message-ID: <CAKywueRd4d_fojGL+n4BisoibhgkYfN9Wyc_+0=-1sarz4-HZw@mail.gmail.com>
Subject: Re: build_path_from_dentry_optional_prefix() may schedule from
 invalid context
To:     Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc:     Steve French <stfrench@microsoft.com>,
        Ronnie Sahlberg <lsahlber@redhat.com>,
        linux-cifs <linux-cifs@vger.kernel.org>,
        Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

=D1=81=D1=80, 28 =D0=B0=D0=B2=D0=B3. 2019 =D0=B3. =D0=B2 22:02, Sergey Seno=
zhatsky
<sergey.senozhatsky.work@gmail.com>:
>
> Hello,
>
> Looking at commit "cifs: create a helper to find a writeable handle
> by path name":
>
> ->open_file_lock scope is atomic context, while build_path_from_dentry()
> can schedule - kmalloc(GFP_KERNEL)
>
>        spin_lock(&tcon->open_file_lock);
>        list_for_each(tmp, &tcon->openFileList) {
>                cfile =3D list_entry(tmp, struct cifsFileInfo,
>                             tlist);
>                full_path =3D build_path_from_dentry(cfile->dentry);
>                if (full_path =3D=3D NULL) {
>                        spin_unlock(&tcon->open_file_lock);
>                        return -ENOMEM;
>                }
>                if (strcmp(full_path, name)) {
>                        kfree(full_path);
>                        continue;
>                }
>                kfree(full_path);
>
>                cinode =3D CIFS_I(d_inode(cfile->dentry));
>                spin_unlock(&tcon->open_file_lock);
>                return cifs_get_writable_file(cinode, 0, ret_file);
>        }
>
>        spin_unlock(&tcon->open_file_lock);
>
> Additionally, kfree() can (and should) be done outside of
> ->open_file_lock scope.
>
>         -ss

Good catch. I think we should have another version of
build_path_from_dentry() which takes pre-allocated (probably on stack)
full_path as an argument. This would allow us to avoid allocations
under the spin lock.
--
Best regards,
Pavel Shilovsky