diff mbox series

[net] net: stmmac: Complete meta data only when enabled

Message ID 20240222-stmmac_xdp-v1-1-e8d2d2b79ff0@linutronix.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] net: stmmac: Complete meta data only when enabled | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 956 this patch: 956
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 16 of 16 maintainers
netdev/build_clang success Errors and warnings before: 973 this patch: 973
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 973 this patch: 973
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-23--03-00 (tests: 1457)

Commit Message

Kurt Kanzenbach Feb. 22, 2024, 9:45 a.m. UTC 
Currently using XDP/ZC sockets on stmmac results in a kernel crash:

|[  255.822584] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
|[...]
|[  255.822764] Call trace:
|[  255.822766]  stmmac_tx_clean.constprop.0+0x848/0xc38

The program counter indicates xsk_tx_metadata_complete(). However, this
function shouldn't be called unless metadata is actually enabled.

Tested on imx93 without XDP, with XDP and with XDP/ZC.

Fixes: 1347b419318d ("net: stmmac: Add Tx HWTS support to XDP ZC")
Suggested-by: Serge Semin <fancer.lancer@gmail.com>
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Link: https://lore.kernel.org/netdev/87r0h7wg8u.fsf@kurt.kurt.home/
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


---
base-commit: 603ead96582d85903baec2d55f021b8dac5c25d2
change-id: 20240222-stmmac_xdp-585ebf1680b3

Best regards,

Comments

Stanislav Fomichev Feb. 22, 2024, 7:53 p.m. UTC | #1 
On Thu, Feb 22, 2024 at 1:45 AM Kurt Kanzenbach <kurt@linutronix.de> wrote:
>
> Currently using XDP/ZC sockets on stmmac results in a kernel crash:
>
> |[  255.822584] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> |[...]
> |[  255.822764] Call trace:
> |[  255.822766]  stmmac_tx_clean.constprop.0+0x848/0xc38
>
> The program counter indicates xsk_tx_metadata_complete(). However, this
> function shouldn't be called unless metadata is actually enabled.
>
> Tested on imx93 without XDP, with XDP and with XDP/ZC.
>
> Fixes: 1347b419318d ("net: stmmac: Add Tx HWTS support to XDP ZC")
> Suggested-by: Serge Semin <fancer.lancer@gmail.com>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> Tested-by: Serge Semin <fancer.lancer@gmail.com>
> Link: https://lore.kernel.org/netdev/87r0h7wg8u.fsf@kurt.kurt.home/

Acked-by: Stanislav Fomichev <sdf@google.com>

LGTM, thanks!

> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index e80d77bd9f1f..8b77c0952071 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2672,7 +2672,8 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int budget, u32 queue,
>                         }
>                         if (skb) {
>                                 stmmac_get_tx_hwtstamp(priv, p, skb);
> -                       } else {
> +                       } else if (tx_q->xsk_pool &&
> +                                  xp_tx_metadata_enabled(tx_q->xsk_pool)) {
>                                 struct stmmac_xsk_tx_complete tx_compl = {
>                                         .priv = priv,
>                                         .desc = p,
>
> ---
> base-commit: 603ead96582d85903baec2d55f021b8dac5c25d2
> change-id: 20240222-stmmac_xdp-585ebf1680b3
>
> Best regards,
> --
> Kurt Kanzenbach <kurt@linutronix.de>
>
Maciej Fijalkowski Feb. 23, 2024, 4:45 p.m. UTC | #2 
On Thu, Feb 22, 2024 at 11:53:14AM -0800, Stanislav Fomichev wrote:
> On Thu, Feb 22, 2024 at 1:45 AM Kurt Kanzenbach <kurt@linutronix.de> wrote:
> >
> > Currently using XDP/ZC sockets on stmmac results in a kernel crash:
> >
> > |[  255.822584] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000

would be good to explain where it comes from, no?

xsk_tx_metadata_to_compl() works on meta == NULL, it does not set
compl->tx_timestamp and later on xsk_tx_metadata_complete() tries to
dereference that.

> > |[...]
> > |[  255.822764] Call trace:
> > |[  255.822766]  stmmac_tx_clean.constprop.0+0x848/0xc38
> >
> > The program counter indicates xsk_tx_metadata_complete(). However, this
> > function shouldn't be called unless metadata is actually enabled.
> >
> > Tested on imx93 without XDP, with XDP and with XDP/ZC.
> >
> > Fixes: 1347b419318d ("net: stmmac: Add Tx HWTS support to XDP ZC")
> > Suggested-by: Serge Semin <fancer.lancer@gmail.com>
> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> > Tested-by: Serge Semin <fancer.lancer@gmail.com>
> > Link: https://lore.kernel.org/netdev/87r0h7wg8u.fsf@kurt.kurt.home/
> 
> Acked-by: Stanislav Fomichev <sdf@google.com>
> 
> LGTM, thanks!
> 
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index e80d77bd9f1f..8b77c0952071 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -2672,7 +2672,8 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int budget, u32 queue,
> >                         }
> >                         if (skb) {
> >                                 stmmac_get_tx_hwtstamp(priv, p, skb);
> > -                       } else {
> > +                       } else if (tx_q->xsk_pool &&
> > +                                  xp_tx_metadata_enabled(tx_q->xsk_pool)) {
> >                                 struct stmmac_xsk_tx_complete tx_compl = {
> >                                         .priv = priv,
> >                                         .desc = p,
> >
> > ---
> > base-commit: 603ead96582d85903baec2d55f021b8dac5c25d2
> > change-id: 20240222-stmmac_xdp-585ebf1680b3
> >
> > Best regards,
> > --
> > Kurt Kanzenbach <kurt@linutronix.de>
> >
Kurt Kanzenbach Feb. 25, 2024, 10:55 a.m. UTC | #3 
Hello Maciej,

On Fri Feb 23 2024, Maciej Fijalkowski wrote:
> On Thu, Feb 22, 2024 at 11:53:14AM -0800, Stanislav Fomichev wrote:
>> On Thu, Feb 22, 2024 at 1:45 AM Kurt Kanzenbach <kurt@linutronix.de> wrote:
>> >
>> > Currently using XDP/ZC sockets on stmmac results in a kernel crash:
>> >
>> > |[  255.822584] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
>
> would be good to explain where it comes from, no?

Sure, no problem.

Thanks,
Kurt
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e80d77bd9f1f..8b77c0952071 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2672,7 +2672,8 @@  static int stmmac_tx_clean(struct stmmac_priv *priv, int budget, u32 queue,
 			}
 			if (skb) {
 				stmmac_get_tx_hwtstamp(priv, p, skb);
-			} else {
+			} else if (tx_q->xsk_pool &&
+				   xp_tx_metadata_enabled(tx_q->xsk_pool)) {
 				struct stmmac_xsk_tx_complete tx_compl = {
 					.priv = priv,
 					.desc = p,