netdev - Re: [PATCH net-next v2] net: txgbe: Add build support for txgbe

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>] [day] [month] [year] [list]

Message-ID: <Yo4ncweml1gRDlhC@lunn.ch>
Date:   Wed, 25 May 2022 14:56:19 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Jiawen Wu <jiawenwu@...stnetic.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2] net: txgbe: Add build support for txgbe

> > > +	/* setup the private structure */
> > > +	err = txgbe_sw_init(adapter);
> > > +	if (err)
> > > +		goto err_sw_init;
> > > +
> > > +	if (pci_using_dac)
> > > +		netdev->features |= NETIF_F_HIGHDMA;
> > 
> > There should probably be a return 0; here, so the probe is successful.
> Without
> > that, you cannot test the remove function.
> > 
> 
> I find that when I execute 'rmmod txgbe', it causes a segmentation fault
> which prints 'iounmap: bad address'.
> But when I try to do 'iounmap' before 'return 0' in the probe function,
> there is no error.
> Could you please tell me the reason for this?

I'm assuming it is this code which is doing the print:

https://elixir.bootlin.com/linux/v5.18/source/arch/x86/mm/ioremap.c#L469

Which suggests the area you are trying to unmap is not actually
mapped.

Your code is a bit confusing:

in probe you have:

+	hw->hw_addr = ioremap(pci_resource_start(pdev, 0),
+			      pci_resource_len(pdev, 0));

and remove:

+	iounmap(adapter->io_addr);

There is an assignment adapter->io_addr = hw->hw_addr; but this is
enough suggestion your structure of adapter and hw is not correct.

What i also notice is that release would normally things in the
opposite order to probe. That is not the case for your code.

> > > +static bool txgbe_check_cfg_remove(struct txgbe_hw *hw, struct
> > > +pci_dev *pdev) {
> > > +	u16 value;
> > > +
> > > +	pci_read_config_word(pdev, PCI_VENDOR_ID, &value);
> > > +	if (value == TXGBE_FAILED_READ_CFG_WORD) {
> > > +		txgbe_remove_adapter(hw);
> > > +		return true;
> > > +	}
> > > +	return false;
> > 
> > This needs a comment to explain what is happening here, because it is not
> > clear to me.
> > 
> 
> It means some kind of problem occur on PCI.

Which does not explain what this function is doing.

It seems like you have two cases to cover:

A PCI problem during probe. This is probably the more likely case. You
just fail the probe.

A PCI problem during run time. What sort of recovery are you going to
do? Just print a warning and keep going, hope for the best? 

    Andrew