netdev - Re: [PATCH v1.2.26] wan: new driver retina

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]

Date:	Fri, 30 May 2008 18:15:55 +0200
From:	Krzysztof Halasa <khc@...waw.pl>
To:	"Matti Linnanvuori" <mattilinn@...il.com>
Cc:	jgarzik@...ox.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1.2.26] wan: new driver retina

"Matti Linnanvuori" <mattilinn@...il.com> writes:

> I would not like to change a legacy driver much.

But this is a new driver, right? I guess the SIOCDEVPRIVATEs will go
at some point so the change can't be really avoided.

>>> +static int fepci_char_open(struct inode *inode, struct file *filp)
>>> +{
>>> +     unsigned int minor = MINOR(inode->i_rdev);
>>> +     if (unlikely(minor >= find_cnt || card_privates[minor].pci_dev == NULL))
>>> +             return -ENXIO;
>>> +     filp->f_op = &fepci_char_fops;
>>> +     if (unlikely(!try_module_get(THIS_MODULE)))
>>> +             return -EBUSY;
>>
>> That won't work race-free, use owner field.
>
> You mean the f_op assignment? I have removed that.

Actually I meant try_module_get() from within the driver, for example
your module may get unloaded while in this function, before this
try_module_get(), and that would be fatal. If you set the owner field
the reference count will be incremented by the caller first.

Though you may be right about the assignement as well.

>>> +static int fepci_stream_open_down(struct net_device *dev,
>>> +                               struct fepci_ch_private *fp)
>>> +{
>>> +     unsigned tx_pages, rx_pages, tx_order, rx_order;
>>> +     unsigned page_number;
>>> +     unsigned int i;
>>> +
>>> +     if (unlikely(fp->in_eth_mode)) {
>>> +             dev_warn(&dev->dev,
>>> +                      "Interface is in Ethernet mode, "
>>> +                      "cannot open stream interface\n");
>>> +             return -EBUSY;
>>> +     }

> Private net device ioctls are locked with rtnl_lock, so I see no
> race there.

Yes, I thought stream mode = character device but now I see it's
another flavour of network device(?), which is never up and exists
only for the ability to receive netdev ioctls (though it also uses
char dev ioctls).

Did I get this right?

Why not simply use a character device, with normal read, write etc?
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html