Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 11 Apr 2023 21:13:43 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Roman Gushchin <roman.gushchin@linux.dev>
Cc:     Lars-Peter Clausen <lars@metafoo.de>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, Rafal Ozieblo <rafalo@cadence.com>
Subject: Re: [PATCH net] net: macb: fix a memory corruption in extended
 buffer descriptor mode
Message-ID: <20230411211343.43b6833a@kernel.org>
In-Reply-To: <ZDYqIj4Fg3tlGKd5@P9FQF9L96D.corp.robot.car>
References: <20230407172402.103168-1-roman.gushchin@linux.dev>
        <20230411184814.5be340a8@kernel.org>
        <6c025530-e2f1-955f-fa5f-8779db23edde@metafoo.de>
        <ZDYqIj4Fg3tlGKd5@P9FQF9L96D.corp.robot.car>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, 11 Apr 2023 20:48:50 -0700 Roman Gushchin wrote:
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c
> > b/drivers/net/ethernet/cadence/macb_main.c
> > index d13fb1d31821..1a40d5a26f36 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -1042,6 +1042,10 @@ static dma_addr_t macb_get_addr(struct macb *bp,
> > struct macb_dma_desc *desc)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
> > =C2=A0#endif
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 addr |=3D MACB_BF(RX_WADDR, =
MACB_BFEXT(RX_WADDR, desc->addr));
> > +#ifdef CONFIG_MACB_USE_HWSTAMP
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (bp->hw_dma_cap & HW_DMA_CAP_P=
TP)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0 addr &=3D ~GEM_BIT(DMA_RXVALID_OFFSET);
> > +#endif
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return addr;
> > =C2=A0}
>=20
> I think this version is slightly worse because it adds an unconditional
> if statement, which can be removed with certain config options.
> I can master a version with a helper function, if it's preferable.
>=20
> But if you like this one, it's fine too, let me know, I'll send an updated
> version.

Yup, IMHO this looks better. More likely that someone reading the code
will spot the trickiness.

I suspect we could clear that bit unconditionally, if the branch is=20
a concern. The code seems to assume that buffers it gets are 8B aligned
already, regardless of CONFIG_MACB_USE_HWSTAMP.

Drivers commonly save the DMA address to a SW ring (here I think
rx_skbuff plays this role but only holds single ptr per entry)
so that they don't have to access potentially uncached descriptor
ring. But that'd be too large of a change for a fix.