linux-kernel - Re: [PATCH v1 1/2] staging: gdm724x: fix type confusion in gdm_lte_event

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: <97e5af3.9183.197cefa02cc.Coremail.linma@zju.edu.cn>
Date: Thu, 3 Jul 2025 14:29:40 +0800 (GMT+08:00)
From: "Lin Ma" <linma@....edu.cn>
To: "Greg KH" <gregkh@...uxfoundation.org>
Cc: wkang77@...il.com, linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org, stable@...nel.org
Subject: Re: [PATCH v1 1/2] staging: gdm724x: fix type confusion in
 gdm_lte_event_rcv()

Hello Greg,

> > @@ -522,6 +522,9 @@ static void gdm_lte_event_rcv(struct net_device *dev, u16 type,
> >  {
> >  	struct nic *nic = netdev_priv(dev);
> >  
> > +	if (dev->netdev_ops->ndo_open != gdm_lte_open)
> > +		return;
> 
> Why should a driver be poking around in netdev_ops?  That feels wrong,
> what would ever change this?  Why not fix that instead?
> 
> thanks,
> 
> greg k-h

Yes, it seems that comparing ops is quite ambiguous and weird. However, investigation on
code shows various checks against net_device.

- by `dev->priv_flags`. See is_vlan_dev()
```
static inline bool is_vlan_dev(const struct net_device *dev)
{
        return dev->priv_flags & IFF_802_1Q_VLAN;
}
```

- by `dev->rtnl_link_ops->kind`. See lookup_interface()
```
static struct wg_device *lookup_interface(struct nlattr **attrs,
                      struct sk_buff *skb)
{
    dev = dev_get_by_index(sock_net(skb->sk),
                       nla_get_u32(attrs[WGDEVICE_A_IFINDEX]));
    ...
    if (!dev->rtnl_link_ops || !dev->rtnl_link_ops->kind ||
        strcmp(dev->rtnl_link_ops->kind, KBUILD_MODNAME)) {
    ...
}
```

- by `dev->type`. See ax25_device_event()
```
static int ax25_device_event(struct notifier_block *this, unsigned long event,
                 void *ptr)
{
    struct net_device *dev = netdev_notifier_info_to_dev(ptr);

    ......

    /* Reject non AX.25 devices */
    if (dev->type != ARPHRD_AX25)
        return NOTIFY_DONE;
```

As for poking around in netdev_ops, we didn't know if it is the best solution,
just because some code did similar checks, e.g.

```
static struct team *team_nl_team_get(struct genl_info *info)
{
    dev = dev_get_by_index(net, ifindex);
    if (!dev || dev->netdev_ops != &team_netdev_ops) {
        ... 
    }
}
```

and

```
static int mlx5e_set_vf_tunnel(...)
{
    route_dev = dev_get_by_index(dev_net(out_dev), route_dev_ifindex);

    if (!route_dev || route_dev->netdev_ops != &mlx5e_netdev_ops || ... ) {
        ...
    }
}
```

From my point of view, using `dev->type/flags/priv_flags` could be a better
choice if it's okay to introduce more definitions.

Using netdev_ops is more straightforward. Or maybe I should just compare the
`->netdev_ops` instead of the `->netdev_ops->ndo_open`?



Thansk
Lin