| Message ID | b2e61a1ac55004ecbbe326cd878cd779df22aae8.1612275417.git.camelia.groza@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dpaa_eth: A050385 erratum workaround fixes under XDP | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 6 maintainers not CCed: madalin.bucur@nxp.com daniel@iogearbox.net john.fastabend@gmail.com bpf@vger.kernel.org ast@kernel.org hawk@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 43 this patch: 43 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 40 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 35 this patch: 35 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Feb 02, 2021 at 07:34:42PM +0200, Camelia Groza wrote: > When the erratum workaround is triggered, the newly created xdp_frame > structure is stored at the start of the newly allocated buffer. Avoid > the structure from being overwritten by explicitly reserving enough > space in the buffer for storing it. > > Account for the fact that the structure's size might increase in time by > aligning the headroom to DPAA_FD_DATA_ALIGNMENT bytes, thus guaranteeing > the data's alignment. > > Fixes: ae680bcbd06a ("dpaa_eth: implement the A050385 erratum workaround for XDP") > Signed-off-by: Camelia Groza <camelia.groza@nxp.com> > --- > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > index 4360ce4d3fb6..e1d041c35ad9 100644 > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > @@ -2182,6 +2182,7 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv, > struct xdp_frame *new_xdpf, *xdpf = *init_xdpf; > void *new_buff; > struct page *p; > + int headroom; > > /* Check the data alignment and make sure the headroom is large > * enough to store the xdpf backpointer. Use an aligned headroom > @@ -2197,19 +2198,31 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv, > return 0; > } > > + /* The new xdp_frame is stored in the new buffer. Reserve enough space > + * in the headroom for storing it along with the driver's private > + * info. The headroom needs to be aligned to DPAA_FD_DATA_ALIGNMENT to > + * guarantee the data's alignment in the buffer. > + */ > + headroom = ALIGN(sizeof(*new_xdpf) + priv->tx_headroom, > + DPAA_FD_DATA_ALIGNMENT); > + > + /* Assure the extended headroom and data fit in a one-paged buffer */ > + if (headroom + xdpf->len > DPAA_BP_RAW_SIZE) This check might make more sense if you would be accounting for skb_shared_info as well I suppose, so that you know you'll still provide enough tailroom for future xdp multibuf support. Didn't all the previous code path make sure that there's a room for that? > + return -ENOMEM; > + > p = dev_alloc_pages(0); > if (unlikely(!p)) > return -ENOMEM; > > /* Copy the data to the new buffer at a properly aligned offset */ > new_buff = page_address(p); > - memcpy(new_buff + priv->tx_headroom, xdpf->data, xdpf->len); > + memcpy(new_buff + headroom, xdpf->data, xdpf->len); > > /* Create an XDP frame around the new buffer in a similar fashion > * to xdp_convert_buff_to_frame. > */ > new_xdpf = new_buff; > - new_xdpf->data = new_buff + priv->tx_headroom; > + new_xdpf->data = new_buff + headroom; > new_xdpf->len = xdpf->len; > new_xdpf->headroom = priv->tx_headroom; > new_xdpf->frame_sz = DPAA_BP_RAW_SIZE; > -- > 2.17.1 >
> -----Original Message----- > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > Sent: Thursday, February 4, 2021 10:52 > To: Camelia Alexandra Groza <camelia.groza@nxp.com> > Cc: kuba@kernel.org; davem@davemloft.net; Madalin Bucur (OSS) > <madalin.bucur@oss.nxp.com>; netdev@vger.kernel.org > Subject: Re: [PATCH net 1/3] dpaa_eth: reserve space for the xdp_frame > under the A050385 erratum > > On Tue, Feb 02, 2021 at 07:34:42PM +0200, Camelia Groza wrote: > > When the erratum workaround is triggered, the newly created xdp_frame > > structure is stored at the start of the newly allocated buffer. Avoid > > the structure from being overwritten by explicitly reserving enough > > space in the buffer for storing it. > > > > Account for the fact that the structure's size might increase in time by > > aligning the headroom to DPAA_FD_DATA_ALIGNMENT bytes, thus > guaranteeing > > the data's alignment. > > > > Fixes: ae680bcbd06a ("dpaa_eth: implement the A050385 erratum > workaround for XDP") > > Signed-off-by: Camelia Groza <camelia.groza@nxp.com> > > --- > > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 17 > +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > index 4360ce4d3fb6..e1d041c35ad9 100644 > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > @@ -2182,6 +2182,7 @@ static int dpaa_a050385_wa_xdpf(struct > dpaa_priv *priv, > > struct xdp_frame *new_xdpf, *xdpf = *init_xdpf; > > void *new_buff; > > struct page *p; > > + int headroom; > > > > /* Check the data alignment and make sure the headroom is large > > * enough to store the xdpf backpointer. Use an aligned headroom > > @@ -2197,19 +2198,31 @@ static int dpaa_a050385_wa_xdpf(struct > dpaa_priv *priv, > > return 0; > > } > > > > + /* The new xdp_frame is stored in the new buffer. Reserve enough > space > > + * in the headroom for storing it along with the driver's private > > + * info. The headroom needs to be aligned to > DPAA_FD_DATA_ALIGNMENT to > > + * guarantee the data's alignment in the buffer. > > + */ > > + headroom = ALIGN(sizeof(*new_xdpf) + priv->tx_headroom, > > + DPAA_FD_DATA_ALIGNMENT); > > + > > + /* Assure the extended headroom and data fit in a one-paged buffer > */ > > + if (headroom + xdpf->len > DPAA_BP_RAW_SIZE) > > This check might make more sense if you would be accounting for > skb_shared_info as well I suppose, so that you know you'll still provide > enough tailroom for future xdp multibuf support. Didn't all the previous > code path make sure that there's a room for that? Hi Maciej This function will have to go through large changes anyway once we enable multibuf support, so making it future-proof isn't a priority. Instead, I do want to return a valid xdp_frame so I need to guarantee the tailroom is there, even if we don't access it . I'll send a v2. Thanks. Camelia
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c index 4360ce4d3fb6..e1d041c35ad9 100644 --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c @@ -2182,6 +2182,7 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv, struct xdp_frame *new_xdpf, *xdpf = *init_xdpf; void *new_buff; struct page *p; + int headroom; /* Check the data alignment and make sure the headroom is large * enough to store the xdpf backpointer. Use an aligned headroom @@ -2197,19 +2198,31 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv, return 0; } + /* The new xdp_frame is stored in the new buffer. Reserve enough space + * in the headroom for storing it along with the driver's private + * info. The headroom needs to be aligned to DPAA_FD_DATA_ALIGNMENT to + * guarantee the data's alignment in the buffer. + */ + headroom = ALIGN(sizeof(*new_xdpf) + priv->tx_headroom, + DPAA_FD_DATA_ALIGNMENT); + + /* Assure the extended headroom and data fit in a one-paged buffer */ + if (headroom + xdpf->len > DPAA_BP_RAW_SIZE) + return -ENOMEM; + p = dev_alloc_pages(0); if (unlikely(!p)) return -ENOMEM; /* Copy the data to the new buffer at a properly aligned offset */ new_buff = page_address(p); - memcpy(new_buff + priv->tx_headroom, xdpf->data, xdpf->len); + memcpy(new_buff + headroom, xdpf->data, xdpf->len); /* Create an XDP frame around the new buffer in a similar fashion * to xdp_convert_buff_to_frame. */ new_xdpf = new_buff; - new_xdpf->data = new_buff + priv->tx_headroom; + new_xdpf->data = new_buff + headroom; new_xdpf->len = xdpf->len; new_xdpf->headroom = priv->tx_headroom; new_xdpf->frame_sz = DPAA_BP_RAW_SIZE;
When the erratum workaround is triggered, the newly created xdp_frame structure is stored at the start of the newly allocated buffer. Avoid the structure from being overwritten by explicitly reserving enough space in the buffer for storing it. Account for the fact that the structure's size might increase in time by aligning the headroom to DPAA_FD_DATA_ALIGNMENT bytes, thus guaranteeing the data's alignment. Fixes: ae680bcbd06a ("dpaa_eth: implement the A050385 erratum workaround for XDP") Signed-off-by: Camelia Groza <camelia.groza@nxp.com> --- drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)