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: <77c96d66-02b9-965d-4c43-c588aedd1d48@linux.intel.com>
Date: Thu, 31 Oct 2024 12:08:20 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: "Zhuo, Qiuxu" <qiuxu.zhuo@...el.com>, 
    Yazen Ghannam <yazen.ghannam@....com>
cc: "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>, 
    "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
    "Luck, Tony" <tony.luck@...el.com>, "x86@...nel.org" <x86@...nel.org>, 
    "avadhut.naik@....com" <avadhut.naik@....com>, 
    "john.allen@....com" <john.allen@....com>, 
    "mario.limonciello@....com" <mario.limonciello@....com>, 
    "bhelgaas@...gle.com" <bhelgaas@...gle.com>, 
    "Shyam-sundar.S-k@....com" <Shyam-sundar.S-k@....com>, 
    "richard.gong@....com" <richard.gong@....com>, 
    "jdelvare@...e.com" <jdelvare@...e.com>, 
    "linux@...ck-us.net" <linux@...ck-us.net>, 
    "clemens@...isch.de" <clemens@...isch.de>, 
    "hdegoede@...hat.com" <hdegoede@...hat.com>, 
    "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>, 
    "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>, 
    "platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>, 
    "naveenkrishna.chatradhi@....com" <naveenkrishna.chatradhi@....com>, 
    "carlos.bilbao.osdev@...il.com" <carlos.bilbao.osdev@...il.com>
Subject: RE: [PATCH 06/16] x86/amd_nb: Simplify root device search

On Thu, 31 Oct 2024, Zhuo, Qiuxu wrote:

> > From: Yazen Ghannam <yazen.ghannam@....com>
> > [...]
> > +struct pci_dev *amd_node_get_root(u16 node) {
> > +	struct pci_dev *df_f0 __free(pci_dev_put) = NULL;
> 
> NULL pointer initialization is not necessary.

It is, because __free() is used...

> > +	struct pci_dev *root;
> > +	u16 cntl_off;
> > +	u8 bus;
> > +
> > +	if (!boot_cpu_has(X86_FEATURE_ZEN))
> > +		return NULL;

...This would try to free() whatever garbage df_f0 holds...

> > +	/*
> > +	 * D18F0xXXX [Config Address Control] (DF::CfgAddressCntl)
> > +	 * Bits [7:0] (SecBusNum) holds the bus number of the root device for
> > +	 * this Data Fabric instance. The segment, device, and function will be
> > 0.
> > +	 */
> > +	df_f0 = amd_node_get_func(node, 0);

...However, the recommended practice when using __free() is this (as 
documented in include/linux/cleanup.h):

 * Given that the "__free(...) = NULL" pattern for variables defined at
 * the top of the function poses this potential interdependency problem
 * the recommendation is to always define and assign variables in one
 * statement and not group variable definitions at the top of the
 * function when __free() is used.

I know the outcome will look undesirable to some, me included, but 
there's little that can be done to that because there's no other way for 
the compiler to infer the order.

That being said, strictly speaking it isn't causing issue in this function 
as is but it's still a bad pattern to initialize to = NULL because in 
other instances it will cause problems. So better to steer away from the
pattern entirely rather than depend on reviewers noticing the a cleaup 
ordering problem gets introduced by some later change to the function.

> > +	if (!df_f0)
> > +		return NULL;


-- 
 i.


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ