Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20231115185439.2616073-1-yzhong@purestorage.com>
 <69218380-45f0-41cc-8a65-50878d44219e@kernel.dk> <CA+AMecFkR0k9ofJt0_iHrPOvfMQ_ePv8QCPYy9m=U7xEjsNCKg@mail.gmail.com>
 <ZVUiLr3g1_i5YmRQ@kbusch-mbp.dhcp.thefacebook.com> <CA+AMecH1TZc3zVm0OF1VwwTBCrLp9mZnOa5zw59yr_W0WbH_uw@mail.gmail.com>
 <ZVWR4Rtr1N7IF6iQ@kbusch-mbp.dhcp.thefacebook.com>
In-Reply-To: <ZVWR4Rtr1N7IF6iQ@kbusch-mbp.dhcp.thefacebook.com>
From:   Yuanyuan Zhong <yzhong@purestorage.com>
Date:   Thu, 16 Nov 2023 11:12:58 -0800
Message-ID: <CA+AMecGH_yugc7YQX2tqMSKzsA-aJYJSVBOVjiyYcw994-_Zng@mail.gmail.com>
Subject: Re: [PATCH] nvme-core: remove head->effects to fix use-after-free
To:     Keith Busch <kbusch@kernel.org>
Cc:     Jens Axboe <axboe@kernel.dk>, hch@lst.de, sagi@grimberg.me,
        linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org,
        randyj@purestorage.com, hcoutinho@purestorage.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Nov 15, 2023 at 7:52=E2=80=AFPM Keith Busch <kbusch@kernel.org> wro=
te:
>
> On Wed, Nov 15, 2023 at 02:44:04PM -0800, Yuanyuan Zhong wrote:
> > On Wed, Nov 15, 2023 at 11:55=E2=80=AFAM Keith Busch <kbusch@kernel.org=
> wrote:
> > >
> > > On Wed, Nov 15, 2023 at 11:21:53AM -0800, Yuanyuan Zhong wrote:
> > > > On Wed, Nov 15, 2023 at 11:02=E2=80=AFAM Jens Axboe <axboe@kernel.d=
k> wrote:
> > > >
> > > > Do you mean something like this? If not, can you please elaborate
> > > > "when we need"?
> > > > -               struct nvme_effects_log *cel =3D xa_load(&ctrl->cel=
s, ns->head->ids.csi);
> > > > +               struct nvme_effects_log *cel =3D (ns->head->ids.csi=
 =3D=3D NVME_CSI_NVM) ?
> > > > +                       ctrl->effects : xa_load(&ctrl->cels, ns->he=
ad->ids.csi);
> > > > Will it be good to change ctrl->effects to ctrl->effects[3] for
> > > > already defined CSI?
> > >
> > > I suggest either re-assign the cached head->effects to one from a sti=
ll
> > > live controller when current path is removed, or move the saved effec=
ts
> > > to the subsystem instead of the controller. All controllers in the
> > > subsystem should be reporting the same effects log anyway, so
> > Is it specified in spec that all controllers in the subsystem
> > should be reporting the same effects log?
>
> Yes, in section 5.16.1.6, "Commands Supported and Effects":
>
>   This log page is used to describe the commands that the controller
>   supports and the effects of those commands on the state of the NVM
>   subsystem.
>
> Oddly enough, Figure 202 says the scope of the log page is "Controller"
> rather than "Subsystem". Sounds like ECN potential. You can memcmp the
> effects log from each controller for a sanity check if you think some
> subsystem controllers messed that up.

Yeah, it says scope is controller.
I think it=E2=80=99s valid to start upgrading one controller in the subsyst=
em
to report different
effects log, e.g. adding or revoking CSUPP for some opcode.
If the saved effects log is kept in the subsystem, how to refresh it
for the subsystem?