linux-kernel - Re: [PATCH 3/3] net: macb: fix _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZJRsYtU4qPZ0h1xp@corigine.com>
Date:   Thu, 22 Jun 2023 17:44:34 +0200
From:   Simon Horman <simon.horman@...igine.com>
To:     Ben Dooks <ben.dooks@...ethink.co.uk>
Cc:     netdev@...r.kernel.org, pabeni@...hat.com, kuba@...nel.org,
        edumazet@...gle.com, davem@...emloft.net,
        linux-kernel@...r.kernel.org, claudiu.beznea@...rochip.com,
        nicolas.ferre@...rochip.com
Subject: Re: [PATCH 3/3] net: macb: fix __be32 warnings in debug code

On Thu, Jun 22, 2023 at 02:05:07PM +0100, Ben Dooks wrote:
> The netdev_dbg() calls in gem_add_flow_filter() and gem_del_flow_filter()
> call ntohl() on the ipv4 addresses, which will put them into the host order
> but not the right type (returns a __be32, not an u32 as would be expected).
> 
> Chaning the htonl() to nthol() should still do the right conversion, return
> the correct u32 type and  should not change any functional to remove the
> following sparse warnings:
> 
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect type in argument 1 (different base types)
> drivers/net/ethernet/cadence/macb_main.c:3568:9:    expected unsigned int [usertype] val
> drivers/net/ethernet/cadence/macb_main.c:3568:9:    got restricted __be32 [usertype] ip4src
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect type in argument 1 (different base types)
> drivers/net/ethernet/cadence/macb_main.c:3568:9:    expected unsigned int [usertype] val
> drivers/net/ethernet/cadence/macb_main.c:3568:9:    got restricted __be32 [usertype] ip4dst
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> d
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect type in argument 1 (different base types)
> drivers/net/ethernet/cadence/macb_main.c:3622:25:    expected unsigned int [usertype] val
> drivers/net/ethernet/cadence/macb_main.c:3622:25:    got restricted __be32 [usertype] ip4src
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect type in argument 1 (different base types)
> drivers/net/ethernet/cadence/macb_main.c:3622:25:    expected unsigned int [usertype] val
> drivers/net/ethernet/cadence/macb_main.c:3622:25:    got restricted __be32 [usertype] ip4dst
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> 
> Signed-off-by: Ben Dooks <ben.dooks@...ethink.co.uk>

Hi Ben,

this code-change looks good to me, but I have a few minor nits for your
consideration.

1. Please specify the target tree, in this case net-next, for patch sets
   for Networking code.

	Subject: [PATCH net-next ...] ...

2. It might be nicer to write '.../macb_main.c' or similar,
   rather tha nthe full path, in the patch description.

3. checkpatch --codespell says: 'Chaning' -> 'Chaining'

> ---
>  drivers/net/ethernet/cadence/macb_main.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 56e202b74bd7..59a90c2b307f 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3568,8 +3568,8 @@ static int gem_add_flow_filter(struct net_device *netdev,
>  	netdev_dbg(netdev,
>  			"Adding flow filter entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n",
>  			fs->flow_type, (int)fs->ring_cookie, fs->location,
> -			htonl(fs->h_u.tcp_ip4_spec.ip4src),
> -			htonl(fs->h_u.tcp_ip4_spec.ip4dst),
> +			ntohl(fs->h_u.tcp_ip4_spec.ip4src),
> +			ntohl(fs->h_u.tcp_ip4_spec.ip4dst),
>  			be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc),
>  			be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst));
>  
> @@ -3622,8 +3622,8 @@ static int gem_del_flow_filter(struct net_device *netdev,
>  			netdev_dbg(netdev,
>  					"Deleting flow filter entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n",
>  					fs->flow_type, (int)fs->ring_cookie, fs->location,
> -					htonl(fs->h_u.tcp_ip4_spec.ip4src),
> -					htonl(fs->h_u.tcp_ip4_spec.ip4dst),
> +					ntohl(fs->h_u.tcp_ip4_spec.ip4src),
> +					ntohl(fs->h_u.tcp_ip4_spec.ip4dst),
>  					be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc),
>  					be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst));
>  
> -- 
> 2.40.1
> 
>