"Revert and fix properly" patch series based on umn.edu re-review

Hi all,

[individuals put on bcc: due to quantity would have been rejected by vger]

Here is the "final" set of reverts and fixes based on the re-review of all accepted umn.edu commits. It consists of 7 "clean" reverts that do not need to be fixed up again for various reasons (see the commit messages for reasoning), and then 31 sets of "revert & fix" commits that consist of reverting the offending commit and then fixing it up properly.

Where these patches were accepted into stable kernels, I've properly tagged them for reverting in the stable kernels automatically as well.

I'll be taking these through one of my trees, so there's no need for any maintainer to have to worry about these needing to go through theirs.

Many thanks to the huge number of people who provided the original "revert review" of all of the patches, as well as the developers here who worked to provide "correct" fixes for these issues so that no kernel release will go out with any bugfix being dropped.

thanks,

greg k-h

----------

Alaa Emad (2):
media: dvb: Add check on sp8870_readreg return
media: gspca: mt9m111: Check write_bridge for timeout

Anirudh Rayabharam (5):
net: fujitsu: fix potential null-ptr-deref net/smc: properly handle workqueue allocation failure
net: stmicro: handle clk_prepare() failure during init
ath6kl: return error code in ath6kl_wmi_set_roam_lrssi_cmd()
rapidio: handle create_workqueue() failure

Atul Gopinathan (3):
serial: max310x: unregister uart driver in case of failure and abort
cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom
ALSA: sb8: Add a comment note regarding an unused pointer

Du Cheng (2):
net: caif: remove BUG_ON(dev == NULL) in caif_xmit
ethernet: sun: niu: fix missing checks of niu_pci_eeprom_read()

Greg Kroah-Hartman (44):
Revert "crypto: cavium/nitrox - add an error message to explain the
failure of pci_request_mem_regions"
Revert "media: rcar_drif: fix a memory disclosure"
Revert "hwmon: (lm80) fix a missing check of bus read in lm80 probe" Revert "serial: mvebu-uart: Fix to avoid a potential NULL pointer
dereference"
Revert "media: usb: gspca: add a missed check for goto_low_power" Revert "ALSA: sb: fix a missing check of snd_ctl_add" Revert "leds: lp5523: fix a missing check of return value of

    lp55xx_read"

Revert "serial: max310x: pass return value of spi_register_driver" Revert "rtlwifi: fix a potential NULL pointer dereference"
net: rtlwifi: properly check for alloc_workqueue() failure Revert "net: fujitsu: fix a potential NULL pointer dereference" Revert "net/smc: fix a NULL pointer dereference"
Revert "net: caif: replace BUG_ON with recovery code" Revert "net: stmicro: fix a missing check of clk_prepare" Revert "niu: fix missing checks of niu_pci_eeprom_read" Revert "qlcnic: Avoid potential NULL pointer dereference" Revert "gdrom: fix a memory leak bug"
Revert "char: hpet: fix a missing check of ioremap"
Revert "scsi: ufs: fix a missing check of devm_reset_control_get" Revert "ALSA: gus: add a check of the status of snd_ctl_add" Revert "ALSA: sb8: add a check for request_region"
Revert "ALSA: usx2y: Fix potential NULL pointer dereference"
ALSA: usx2y: check for failure of usb_alloc_urb()
Revert "video: hgafb: fix potential NULL pointer dereference" Revert "isdn: mISDNinfineon: fix potential NULL pointer dereference" Revert "ath6kl: return error code in ath6kl_wmi_set_roam_lrssi_cmd()" Revert "rapidio: fix a NULL pointer dereference when
create_workqueue() fails"
Revert "isdn: mISDN: Fix potential NULL pointer dereference of
kzalloc"
Revert "ecryptfs: replace BUG_ON with error handling code" Revert "dmaengine: qcom_hidma: Check for driver register failure" Revert "libertas: add checks for the return value of
sysfs_create_group"
libertas: register sysfs groups properly
Revert "ASoC: rt5645: fix a NULL pointer dereference" Revert "ASoC: cs43130: fix a NULL pointer dereference"
ASoC: cs43130: handle errors in cs43130_probe() properly Revert "media: dvb: Add check on sp8870_readreg"
Revert "media: gspca: mt9m111: Check write_bridge for timeout" Revert "media: gspca: Check the return value of write_bridge for
timeout"
media: gspca: properly check for errors in po1030_probe() Revert "net: liquidio: fix a NULL pointer dereference" Revert "video: imsttfb: fix potential NULL pointer dereferences"
video: imsttfb: check for ioremap() failures
Revert "brcmfmac: add a check for the status of usb_register"
brcmfmac: properly check for bus register errors

Igor Matheus Andrade Torrente (1):
video: hgafb: fix potential NULL pointer dereference

Kees Cook (1):
Revert "ACPI: custom_method: fix memory leaks"

Kurt Manucredo (1):
ALSA: gus: properly handle snd_ctl_add() error

Phillip Potter (7):
leds: lp5523: check return value of lp5xx_read and jump to cleanup
code
scsi: ufs: handle cleanup correctly on devm_reset_control_get error
isdn: mISDNinfineon: check/cleanup ioremap failure correctly in
setup_io
isdn: mISDN: correctly handle ph_info allocation failure in
hfcsusb_ph_info
fs: ecryptfs: remove BUG_ON from crypt_scatterlist
dmaengine: qcom_hidma: comment platform_driver_register call
ASoC: rt5645: add error checking to rt5645_probe function

Tom Seewald (3):
qlcnic: Add null check after calling netdev_alloc_skb
char: hpet: add checks after calling ioremap
net: liquidio: Add missing null pointer checks

 drivers/acpi/custom_method.c                  |  5 +-
 drivers/cdrom/gdrom.c                         |  4 +-
 drivers/char/hpet.c                           |  2 +
 drivers/crypto/cavium/nitrox/nitrox_main.c    |  1 -
 drivers/dma/qcom/hidma_mgmt.c                 | 17 ++++++-
 drivers/hwmon/lm80.c                          | 11 +----
 drivers/isdn/hardware/mISDN/hfcsusb.c         | 17 +++----
 drivers/isdn/hardware/mISDN/mISDNinfineon.c   | 21 +++++---
 drivers/leds/leds-lp5523.c                    |  2 +-
 drivers/media/dvb-frontends/sp8870.c          |  2 +-
 drivers/media/platform/rcar_drif.c            |  1 -
 drivers/media/usb/gspca/cpia1.c               |  6 +--
 drivers/media/usb/gspca/m5602/m5602_mt9m111.c | 16 +++---
 drivers/media/usb/gspca/m5602/m5602_po1030.c  | 14 +++---
 drivers/net/caif/caif_serial.c                |  3 --
 .../net/ethernet/cavium/liquidio/lio_main.c   | 27 ++++++----
 .../ethernet/cavium/liquidio/lio_vf_main.c    | 27 +++++++---
 drivers/net/ethernet/fujitsu/fmvj18x_cs.c     |  4 +-
 .../ethernet/qlogic/qlcnic/qlcnic_ethtool.c   |  3 +-
 .../net/ethernet/stmicro/stmmac/dwmac-sunxi.c |  8 +--
 drivers/net/ethernet/sun/niu.c                | 32 +++++++-----
 drivers/net/wireless/ath/ath6kl/debug.c       |  5 +-
 .../broadcom/brcm80211/brcmfmac/bcmsdh.c      |  8 +--
 .../broadcom/brcm80211/brcmfmac/bus.h         | 19 ++++++-
 .../broadcom/brcm80211/brcmfmac/core.c        | 42 +++++++---------
 .../broadcom/brcm80211/brcmfmac/pcie.c        |  9 +---
 .../broadcom/brcm80211/brcmfmac/pcie.h        |  5 --
 .../broadcom/brcm80211/brcmfmac/usb.c         |  8 +--
 drivers/net/wireless/marvell/libertas/mesh.c  | 33 ++-----------
 drivers/net/wireless/realtek/rtlwifi/base.c   | 18 +++----
 drivers/rapidio/rio_cm.c                      | 17 +++----
 drivers/scsi/ufs/ufs-hisi.c                   | 15 +++---
 drivers/tty/serial/max310x.c                  |  2 +
 drivers/tty/serial/mvebu-uart.c               |  3 --
 drivers/video/fbdev/hgafb.c                   | 21 ++++----
 drivers/video/fbdev/imsttfb.c                 | 26 +++++++---
 fs/ecryptfs/crypto.c                          |  4 --
 net/smc/smc_ism.c                             | 15 +++---
 sound/isa/gus/gus_main.c                      | 19 ++-----
 sound/isa/sb/sb16_main.c                      | 10 ++--
 sound/isa/sb/sb8.c                            | 10 ++--
 sound/soc/codecs/cs43130.c                    | 28 +++++------
 sound/soc/codecs/rt5645.c                     | 49 ++++++++++++++-----
 sound/usb/usx2y/usb_stream.c                  | 22 +++++++--
 44 files changed, 323 insertions(+), 288 deletions(-)

2.31.1

This reverts commit 9fcddaf2e28d779cb946d23838ba6d50f299aa80 as it was submitted under a fake name and we can not knowingly accept anonymous contributions to the repository.

This commit was part of a submission "test" to the Linux kernel community by some "researchers" at umn.edu. As outlined at:
https://www-users.cs.umn.edu/%7Ekjlu/papers/full-disclosure.pdf
it was done so as an attempt to submit a known-buggy patch to see if it could get by our review. However, the submission turned out to actually be correct, and not have a bug in it as the authors did not understand how the PCI driver model works at all, and so the submission was accepted.

As this change is of useless consequence, there is no loss of functionality in reverting it.

---
 drivers/crypto/cavium/nitrox/nitrox_main.c | 1 -
 1 file changed, 1 deletion(-)

index facc8e6bc580..d385daf2c71c 100644
@@ -442,7 +442,6 @@ static int nitrox_probe(struct pci_dev *pdev,
 	err = pci_request_mem_regions(pdev, nitrox_driver_name);
 	if (err) {
 		pci_disable_device(pdev);
-		dev_err(&pdev->dev, "Failed to request mem regions!\n");
 		return err;
 	}
 	pci_set_master(pdev);

2.31.1

From: Kees Cook <[email protected]>

This reverts commit 03d1571d9513369c17e6848476763ebbd10ec2cb.

While /sys/kernel/debug/acpi/custom_method is already a privileged-only API providing proxied arbitrary write access to kernel memory[1][2], with existing race conditions[3] in buffer allocation and use that could lead to memory leaks and use-after-free conditions, the above commit appears to accidentally make the use-after-free conditions even easier to accomplish. ("buf" is a global variable and prior kfree()s would set buf back to NULL.)

This entire interface needs to be reworked (if not entirely removed).

[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
[3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/

---
 drivers/acpi/custom_method.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index 443fdf62dd22..72469a49837d 100644
@@ -53,10 +53,8 @@ static ssize_t cm_write(struct file *file, const char __user *user_buf,
 	if ((*ppos > max_size) ||
 	    (*ppos + count > max_size) ||
 	    (*ppos + count < count) ||
-	    (count > uncopied_bytes)) {
-		kfree(buf);
+	    (count > uncopied_bytes))
 		return -EINVAL;
-	}
 
 	if (copy_from_user(buf + (*ppos), user_buf, count)) {
 		kfree(buf);

@@ -76,7 +74,6 @@ static ssize_t cm_write(struct file *file, const char __user *user_buf,
 		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, LOCKDEP_NOW_UNRELIABLE);
 	}
 
-	kfree(buf);
 	return count;
 }
 

2.31.1

This reverts commit d39083234c60519724c6ed59509a2129fd2aed41.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, it was determined that this commit is not needed at all as the media core already prevents memory disclosure on this codepath, so just drop the extra memset happening here.

---
 drivers/media/platform/rcar_drif.c | 1 -
 1 file changed, 1 deletion(-)

index 83bd9a412a56..1e3b68a8743a 100644
@@ -915,7 +915,6 @@ static int rcar_drif_g_fmt_sdr_cap(struct file *file, void *priv,
 {
 	struct rcar_drif_sdr *sdr = video_drvdata(file);
 
-	memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
 	f->fmt.sdr.pixelformat = sdr->fmt->pixelformat;
 	f->fmt.sdr.buffersize = sdr->fmt->buffersize;
 

2.31.1

This reverts commit 9aa3aa15f4c2f74f47afd6c5db4b420fadf3f315.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, it was determined that this commit is not needed at all so just revert it. Also, the call to lm80_init_client() was not properly handled, so if error handling is needed in the lm80_probe() function, then it should be done properly, not half-baked like the commit being reverted here did.

---
 drivers/hwmon/lm80.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

index ac4adb44b224..97ab491d2922 100644
@@ -596,7 +596,6 @@ static int lm80_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
 	struct lm80_data *data;
-	int rv;
 
 	data = devm_kzalloc(dev, sizeof(struct lm80_data), GFP_KERNEL);
 	if (!data)

@@ -609,14 +608,8 @@ static int lm80_probe(struct i2c_client *client)
 	lm80_init_client(client);
 
 	/* A few vars need to be filled upon startup */
-	rv = lm80_read_value(client, LM80_REG_FAN_MIN(1));
-	if (rv < 0)
-		return rv;
-	data->fan[f_min][0] = rv;
-	rv = lm80_read_value(client, LM80_REG_FAN_MIN(2));
-	if (rv < 0)
-		return rv;
-	data->fan[f_min][1] = rv;
+	data->fan[f_min][0] = lm80_read_value(client, LM80_REG_FAN_MIN(1));
+	data->fan[f_min][1] = lm80_read_value(client, LM80_REG_FAN_MIN(2));
 
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
 							   data, lm80_groups);

2.31.1

This reverts commit 32f47179833b63de72427131169809065db6745e.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be not be needed at all as the change was useless because this function can only be called when of_match_device matched on something. So it should be reverted.

---
 drivers/tty/serial/mvebu-uart.c | 3 ---
 1 file changed, 3 deletions(-)

index e0c00a1b0763..51b0ecabf2ec 100644
@@ -818,9 +818,6 @@ static int mvebu_uart_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	if (!match)
-		return -ENODEV;
-
 	/* Assume that all UART ports have a DT alias or none has */
 	id = of_alias_get_id(pdev->dev.of_node, "serial");
 	if (!pdev->dev.of_node || id < 0)

2.31.1

This reverts commit 5b711870bec4dc9a6d705d41e127e73944fa3650.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to do does nothing useful as a user can do nothing with this information and if an error did happen, the code would continue on as before. Because of this, just revert it.

---
 drivers/media/usb/gspca/cpia1.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

index a4f7431486f3..d93d384286c1 100644
@@ -1424,7 +1424,6 @@ static int sd_config(struct gspca_dev *gspca_dev,
 {
 	struct sd *sd = (struct sd *) gspca_dev;
 	struct cam *cam;
-	int ret;
 
 	sd->mainsFreq = FREQ_DEF == V4L2_CID_POWER_LINE_FREQUENCY_60HZ;
 	reset_camera_params(gspca_dev);

@@ -1436,10 +1435,7 @@ static int sd_config(struct gspca_dev *gspca_dev,
 	cam->cam_mode = mode;
 	cam->nmodes = ARRAY_SIZE(mode);
 
-	ret = goto_low_power(gspca_dev);
-	if (ret)
-		gspca_err(gspca_dev, "Cannot go to low power mode: %d\n",
-			  ret);
+	goto_low_power(gspca_dev);
 	/* Check the firmware version. */
 	sd->params.version.firmwareVersion = 0;
 	get_version_information(gspca_dev);

2.31.1

This reverts commit beae77170c60aa786f3e4599c18ead2854d8694d.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It is safe to ignore this error as the mixer element is optional, and the driver is very legacy.

---
 sound/isa/sb/sb16_main.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

index 38dc1fde25f3..aa4870531023 100644
@@ -846,14 +846,10 @@ int snd_sb16dsp_pcm(struct snd_sb *chip, int device)
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &snd_sb16_playback_ops);
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &snd_sb16_capture_ops);
 
-	if (chip->dma16 >= 0 && chip->dma8 != chip->dma16) {
-		err = snd_ctl_add(card, snd_ctl_new1(
-					&snd_sb16_dma_control, chip));
-		if (err)
-			return err;
-	} else {
+	if (chip->dma16 >= 0 && chip->dma8 != chip->dma16)
+		snd_ctl_add(card, snd_ctl_new1(&snd_sb16_dma_control, chip));
+	else
 		pcm->info_flags = SNDRV_PCM_INFO_HALF_DUPLEX;
-	}
 
 	snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV,
 				       card->dev, 64*1024, 128*1024);

2.31.1

This reverts commit 248b57015f35c94d4eae2fdd8c6febf5cd703900.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original commit does not properly unwind if there is an error condition so it needs to be reverted at this point in time.

---
 drivers/leds/leds-lp5523.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index fc433e63b1dc..5036d7d5f3d4 100644
@@ -305,9 +305,7 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
 
 	/* Let the programs run for couple of ms and check the engine status */
 	usleep_range(3000, 6000);
-	ret = lp55xx_read(chip, LP5523_REG_STATUS, &status);
-	if (ret)
-		return ret;
+	lp55xx_read(chip, LP5523_REG_STATUS, &status);
 	status &= LP5523_ENG_STATUS_MASK;
 
 	if (status != LP5523_ENG_STATUS_MASK) {

2.31.1

From: Phillip Potter <[email protected]>

Check return value of lp5xx_read and if non-zero, jump to code at end of the function, causing lp5523_stop_all_engines to be executed before returning the error value up the call chain. This fixes the original commit (248b57015f35) which was reverted due to the University of Minnesota problems.

---
 drivers/leds/leds-lp5523.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 5036d7d5f3d4..b1590cb4a188 100644
@@ -305,7 +305,9 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
 
 	/* Let the programs run for couple of ms and check the engine status */
 	usleep_range(3000, 6000);
-	lp55xx_read(chip, LP5523_REG_STATUS, &status);
+	ret = lp55xx_read(chip, LP5523_REG_STATUS, &status);
+	if (ret)
+		goto out;
 	status &= LP5523_ENG_STATUS_MASK;
 
 	if (status != LP5523_ENG_STATUS_MASK) {

2.31.1

Thanks for the review!

This reverts commit 51f689cc11333944c7a457f25ec75fcb41e99410.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

This change did not properly unwind from the error condition, so it was not correct.

---
 drivers/tty/serial/max310x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 8534d6e45a1d..a3ba0e6520a1 100644
@@ -1518,10 +1518,10 @@ static int __init max310x_uart_init(void)
 		return ret;
 
 #ifdef CONFIG_SPI_MASTER
-	ret = spi_register_driver(&max310x_spi_driver);
+	spi_register_driver(&max310x_spi_driver);
 #endif
 
-	return ret;
+	return 0;
 }
 module_init(max310x_uart_init);
 

2.31.1

From: Atul Gopinathan <[email protected]>

The macro "spi_register_driver" invokes the function "__spi_register_driver()" which has a return type of int and can fail, returning a negative value in such a case. This is currently ignored and the init() function yields success even if the spi driver failed to register.

Fix this by collecting the return value of "__spi_register_driver()" and also unregister the uart driver in case of failure.

---
 drivers/tty/serial/max310x.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index a3ba0e6520a1..3cbc757d7be7 100644
@@ -1518,10 +1518,12 @@ static int __init max310x_uart_init(void)
 		return ret;
 
 #ifdef CONFIG_SPI_MASTER
-	spi_register_driver(&max310x_spi_driver);
+	ret = spi_register_driver(&max310x_spi_driver);
+	if (ret)
+		uart_unregister_driver(&max310x_uart);
 #endif
 
-	return 0;
+	return ret;
 }
 module_init(max310x_uart_init);
 

2.31.1

This reverts commit 765976285a8c8db3f0eb7f033829a899d0c2786e.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

This commit is not correct, it should not have used unlikely() and is not propagating the error properly to the calling function, so it should be reverted at this point in time. Also, if the check failed, the work queue was still assumed to be allocated, so further accesses would have continued to fail, meaning this patch does nothing to solve the root issues at all.

---
 drivers/net/wireless/realtek/rtlwifi/base.c | 5 -----
 1 file changed, 5 deletions(-)

index 2a7ee90a3f54..4136d7c63254 100644
@@ -452,11 +452,6 @@ static void _rtl_init_deferred_work(struct ieee80211_hw *hw)
 	/* <2> work queue */
 	rtlpriv->works.hw = hw;
 	rtlpriv->works.rtl_wq = alloc_workqueue("%s", 0, 0, rtlpriv->cfg->name);
-	if (unlikely(!rtlpriv->works.rtl_wq)) {
-		pr_err("Failed to allocate work queue\n");
-		return;
-	}
-
 	INIT_DELAYED_WORK(&rtlpriv->works.watchdog_wq,
 			  rtl_watchdog_wq_callback);
 	INIT_DELAYED_WORK(&rtlpriv->works.ips_nic_off_wq,

2.31.1

If alloc_workqueue() fails, properly catch this and propagate the error to the calling functions, so that the devuce initialization will properly error out.

---
 drivers/net/wireless/realtek/rtlwifi/base.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

index 4136d7c63254..ffd150ec181f 100644
@@ -440,9 +440,14 @@ static void rtl_watchdog_wq_callback(struct work_struct *work);
 static void rtl_fwevt_wq_callback(struct work_struct *work);
 static void rtl_c2hcmd_wq_callback(struct work_struct *work);
 
-static void _rtl_init_deferred_work(struct ieee80211_hw *hw)
+static int _rtl_init_deferred_work(struct ieee80211_hw *hw)
 {
 	struct rtl_priv *rtlpriv = rtl_priv(hw);
+	struct workqueue_struct *wq;
+
+	wq = alloc_workqueue("%s", 0, 0, rtlpriv->cfg->name);
+	if (!wq)
+		return -ENOMEM;
 
 	/* <1> timer */
 	timer_setup(&rtlpriv->works.watchdog_timer,

@@ -451,7 +456,8 @@ static void _rtl_init_deferred_work(struct ieee80211_hw *hw)
 		    rtl_easy_concurrent_retrytimer_callback, 0);
 	/* <2> work queue */
 	rtlpriv->works.hw = hw;
-	rtlpriv->works.rtl_wq = alloc_workqueue("%s", 0, 0, rtlpriv->cfg->name);
+	rtlpriv->works.rtl_wq = wq;
+
 	INIT_DELAYED_WORK(&rtlpriv->works.watchdog_wq,
 			  rtl_watchdog_wq_callback);
 	INIT_DELAYED_WORK(&rtlpriv->works.ips_nic_off_wq,

@@ -461,6 +467,7 @@ static void _rtl_init_deferred_work(struct ieee80211_hw *hw)
 			  rtl_swlps_rfon_wq_callback);
 	INIT_DELAYED_WORK(&rtlpriv->works.fwevt_wq, rtl_fwevt_wq_callback);
 	INIT_DELAYED_WORK(&rtlpriv->works.c2hcmd_wq, rtl_c2hcmd_wq_callback);
+	return 0;
 }
 
 void rtl_deinit_deferred_work(struct ieee80211_hw *hw, bool ips_wq)

@@ -559,9 +566,7 @@ int rtl_init_core(struct ieee80211_hw *hw)
 	rtlmac->link_state = MAC80211_NOLINK;
 
 	/* <6> init deferred work */
-	_rtl_init_deferred_work(hw);
-
-	return 0;
+	return _rtl_init_deferred_work(hw);
 }
 EXPORT_SYMBOL_GPL(rtl_init_core);
 

2.31.1

This reverts commit 9f4d6358e11bbc7b839f9419636188e4151fb6e4.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original change does not change any behavior as the caller of this function onlyu checks for "== -1" as an error condition so this error is not handled properly. Remove this change and it will be fixed up properly in a later commit.

---
 drivers/net/ethernet/fujitsu/fmvj18x_cs.c | 5 -----
 1 file changed, 5 deletions(-)

index a7b7a4aace79..dc90c61fc827 100644
@@ -547,11 +547,6 @@ static int fmvj18x_get_hwinfo(struct pcmcia_device *link, u_char *node_id)
 	return -1;
 
     base = ioremap(link->resource[2]->start, resource_size(link->resource[2]));
-    if (!base) {
-	    pcmcia_release_window(link, link->resource[2]);
-	    return -ENOMEM;
-    }
-
     pcmcia_map_mem_page(link, link->resource[2], 0);
 
     /*

2.31.1

From: Anirudh Rayabharam <[email protected]>

In fmvj18x_get_hwinfo(), if ioremap fails there will be NULL pointer deref. To fix this, check the return value of ioremap and return -1 to the caller in case of failure.

---
 drivers/net/ethernet/fujitsu/fmvj18x_cs.c | 5 +++++
 1 file changed, 5 insertions(+)

index dc90c61fc827..b0c0504950d8 100644
@@ -547,6 +547,11 @@ static int fmvj18x_get_hwinfo(struct pcmcia_device *link, u_char *node_id)
 	return -1;
 
     base = ioremap(link->resource[2]->start, resource_size(link->resource[2]));
+    if (!base) {
+	pcmcia_release_window(link, link->resource[2]);
+	return -1;
+    }
+
     pcmcia_map_mem_page(link, link->resource[2], 0);
 
     /*

2.31.1

This reverts commit e183d4e414b64711baf7a04e214b61969ca08dfa.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original commit causes a memory leak and does not properly fix the issue it claims to fix. I will send a follow-on patch to resolve this properly.

---
 net/smc/smc_ism.c | 5 -----
 1 file changed, 5 deletions(-)

index 9c6e95882553..6558cf7643a7 100644
@@ -417,11 +417,6 @@ struct smcd_dev *smcd_alloc_dev(struct device *parent, const char *name,
 	init_waitqueue_head(&smcd->lgrs_deleted);
 	smcd->event_wq = alloc_ordered_workqueue("ism_evt_wq-%s)",
 						 WQ_MEM_RECLAIM, name);
-	if (!smcd->event_wq) {
-		kfree(smcd->conn);
-		kfree(smcd);
-		return NULL;
-	}
 	return smcd;
 }
 EXPORT_SYMBOL_GPL(smcd_alloc_dev);

2.31.1

From: Anirudh Rayabharam <[email protected]>

In smcd_alloc_dev(), if alloc_ordered_workqueue() fails, properly catch it, clean up and return NULL to let the caller know there was a failure. Move the call to alloc_ordered_workqueue higher in the function in order to abort earlier without needing to unwind the call to device_initialize().

---
 net/smc/smc_ism.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index 6558cf7643a7..94b31f2551bc 100644
@@ -402,6 +402,14 @@ struct smcd_dev *smcd_alloc_dev(struct device *parent, const char *name,
 		return NULL;
 	}
 
+	smcd->event_wq = alloc_ordered_workqueue("ism_evt_wq-%s)",
+						 WQ_MEM_RECLAIM, name);
+	if (!smcd->event_wq) {
+		kfree(smcd->conn);
+		kfree(smcd);
+		return NULL;
+	}
+
 	smcd->dev.parent = parent;
 	smcd->dev.release = smcd_release;
 	device_initialize(&smcd->dev);

@@ -415,8 +423,6 @@ struct smcd_dev *smcd_alloc_dev(struct device *parent, const char *name,
 	INIT_LIST_HEAD(&smcd->vlan);
 	INIT_LIST_HEAD(&smcd->lgr_list);
 	init_waitqueue_head(&smcd->lgrs_deleted);
-	smcd->event_wq = alloc_ordered_workqueue("ism_evt_wq-%s)",
-						 WQ_MEM_RECLAIM, name);
 	return smcd;
 }
 EXPORT_SYMBOL_GPL(smcd_alloc_dev);

2.31.1

This reverts commit c5dea815834c7d2e9fc633785455bc428b7a1956.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original change here was pointless as dev can never be NULL in this function so the claim in the changelog that this "fixes" anything is incorrect (also the developer forgot about panic_on_warn). A follow-up change will resolve this issue properly.

---
 drivers/net/caif/caif_serial.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index da6fffb4d5a8..a7f51eb58915 100644
@@ -269,9 +269,7 @@ static netdev_tx_t caif_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct ser_device *ser;
 
-	if (WARN_ON(!dev))
-		return -EINVAL;
-
+	BUG_ON(dev == NULL);
 	ser = netdev_priv(dev);
 
 	/* Send flow off once, on high water mark */

2.31.1

From: Du Cheng <[email protected]>

The condition of dev == NULL is impossible in caif_xmit(), hence it is for the removal.

Explanation:
The static caif_xmit() is only called upon via a function pointer ndo_start_xmit defined in include/linux/netdevice.h:

```
struct net_device_ops {
    ...
    netdev_tx_t     (*ndo_start_xmit)(struct sk_buff *skb, struct net_device *dev);
    ...
}
```

The exhausive list of call points are:

```
drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
    dev->netdev_ops->ndo_start_xmit(skb, dev);
    ^                                    ^

drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c
    struct opa_vnic_adapter *adapter = opa_vnic_priv(netdev);
			     ^                       ^
    return adapter->rn_ops->ndo_start_xmit(skb, netdev); // adapter would crash first
	   ^                                    ^

drivers/usb/gadget/function/f_ncm.c
    ncm->netdev->netdev_ops->ndo_start_xmit(NULL, ncm->netdev);
	      ^                                   ^

include/linux/netdevice.h
static inline netdev_tx_t __netdev_start_xmit(...
{
    return ops->ndo_start_xmit(skb, dev);
				    ^
}

    const struct net_device_ops *ops = dev->netdev_ops;
				       ^
    rc = __netdev_start_xmit(ops, skb, dev, more);
				       ^
```

In each of the enumerated scenarios, it is impossible for the NULL-valued dev to reach the caif_xmit() without crashing the kernel earlier, therefore BUG_ON(dev == NULL) is rather useless, hence the removal.

---
 drivers/net/caif/caif_serial.c | 1 -
 1 file changed, 1 deletion(-)

index a7f51eb58915..d17482395a4d 100644
@@ -269,7 +269,6 @@ static netdev_tx_t caif_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct ser_device *ser;
 
-	BUG_ON(dev == NULL);
 	ser = netdev_priv(dev);
 
 	/* Send flow off once, on high water mark */

2.31.1

This reverts commit f86a3b83833e7cfe558ca4d70b64ebc48903efec.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original commit causes a memory leak when it is trying to claim it is properly handling errors. Revert this change and fix it up properly in a follow-on commit.

---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index 527077c98ebc..fc68e90acbea 100644
@@ -50,9 +50,7 @@ static int sun7i_gmac_init(struct platform_device *pdev, void *priv)
 		gmac->clk_enabled = 1;
 	} else {
 		clk_set_rate(gmac->tx_clk, SUN7I_GMAC_MII_RATE);
-		ret = clk_prepare(gmac->tx_clk);
-		if (ret)
-			return ret;
+		clk_prepare(gmac->tx_clk);
 	}
 
 	return 0;

2.31.1

From: Anirudh Rayabharam <[email protected]>

In case clk_prepare() fails, capture and propagate the error code up the stack. If regulator_enable() was called earlier, properly unwind it by calling regulator_disable().

---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

index fc68e90acbea..fc3b0acc8f99 100644
@@ -30,7 +30,7 @@ struct sunxi_priv_data {
 static int sun7i_gmac_init(struct platform_device *pdev, void *priv)
 {
 	struct sunxi_priv_data *gmac = priv;
-	int ret;
+	int ret = 0;
 
 	if (gmac->regulator) {
 		ret = regulator_enable(gmac->regulator);

@@ -50,10 +50,12 @@ static int sun7i_gmac_init(struct platform_device *pdev, void *priv)
 		gmac->clk_enabled = 1;
 	} else {
 		clk_set_rate(gmac->tx_clk, SUN7I_GMAC_MII_RATE);
-		clk_prepare(gmac->tx_clk);
+		ret = clk_prepare(gmac->tx_clk);
+		if (ret && gmac->regulator)
+			regulator_disable(gmac->regulator);
 	}
 
-	return 0;
+	return ret;
 }
 
 static void sun7i_gmac_exit(struct platform_device *pdev, void *priv)

2.31.1

This reverts commit 26fd962bde0b15e54234fe762d86bc0349df1de4.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The change here was incorrect. While it is nice to check if niu_pci_eeprom_read() succeeded or not when using the data, any error that might have happened was not propagated upwards properly, causing the kernel to assume that these reads were successful, which results in invalid data in the buffer that was to contain the successfully read data.

---
 drivers/net/ethernet/sun/niu.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

index 707ccdd03b19..d70cdea756d1 100644
@@ -8097,8 +8097,6 @@ static int niu_pci_vpd_scan_props(struct niu *np, u32 start, u32 end)
 		start += 3;
 
 		prop_len = niu_pci_eeprom_read(np, start + 4);
-		if (prop_len < 0)
-			return prop_len;
 		err = niu_pci_vpd_get_propname(np, start + 5, namebuf, 64);
 		if (err < 0)
 			return err;

@@ -8143,12 +8141,8 @@ static int niu_pci_vpd_scan_props(struct niu *np, u32 start, u32 end)
 			netif_printk(np, probe, KERN_DEBUG, np->dev,
 				     "VPD_SCAN: Reading in property [%s] len[%d]\n",
 				     namebuf, prop_len);
-			for (i = 0; i < prop_len; i++) {
-				err = niu_pci_eeprom_read(np, off + i);
-				if (err >= 0)
-					*prop_buf = err;
-				++prop_buf;
-			}
+			for (i = 0; i < prop_len; i++)
+				*prop_buf++ = niu_pci_eeprom_read(np, off + i);
 		}
 
 		start += len;

2.31.1

From: Du Cheng <[email protected]>

niu_pci_eeprom_read() may fail, so add checks to its return value and propagate the error up the callstack.

An examination of the callstack up to niu_pci_eeprom_read shows that:

niu_pci_eeprom_read() // returns int
niu_pci_vpd_scan_props() // returns int
niu_pci_vpd_fetch() // returns *void*
niu_get_invariants() // returns int

since niu_pci_vpd_fetch() returns void which breaks the bubbling up, change its return type to int so that error is propagated upwards.

---
 drivers/net/ethernet/sun/niu.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

index d70cdea756d1..74e748662ec0 100644
@@ -8097,6 +8097,8 @@ static int niu_pci_vpd_scan_props(struct niu *np, u32 start, u32 end)
 		start += 3;
 
 		prop_len = niu_pci_eeprom_read(np, start + 4);
+		if (prop_len < 0)
+			return prop_len;
 		err = niu_pci_vpd_get_propname(np, start + 5, namebuf, 64);
 		if (err < 0)
 			return err;

@@ -8141,8 +8143,12 @@ static int niu_pci_vpd_scan_props(struct niu *np, u32 start, u32 end)
 			netif_printk(np, probe, KERN_DEBUG, np->dev,
 				     "VPD_SCAN: Reading in property [%s] len[%d]\n",
 				     namebuf, prop_len);
-			for (i = 0; i < prop_len; i++)
-				*prop_buf++ = niu_pci_eeprom_read(np, off + i);
+			for (i = 0; i < prop_len; i++) {
+				err =  niu_pci_eeprom_read(np, off + i);
+				if (err < 0)
+					return err;
+				*prop_buf++ = err;
+			}
 		}
 
 		start += len;

@@ -8152,14 +8158,14 @@ static int niu_pci_vpd_scan_props(struct niu *np, u32 start, u32 end)
 }
 
 /* ESPC_PIO_EN_ENABLE must be set */
-static void niu_pci_vpd_fetch(struct niu *np, u32 start)
+static int niu_pci_vpd_fetch(struct niu *np, u32 start)
 {
 	u32 offset;
 	int err;
 
 	err = niu_pci_eeprom_read16_swp(np, start + 1);
 	if (err < 0)
-		return;
+		return err;
 
 	offset = err + 3;
 

@@ -8168,12 +8174,14 @@ static void niu_pci_vpd_fetch(struct niu *np, u32 start)
 		u32 end;
 
 		err = niu_pci_eeprom_read(np, here);
+		if (err < 0)
+			return err;
 		if (err != 0x90)
-			return;
+			return -EINVAL;
 
 		err = niu_pci_eeprom_read16_swp(np, here + 1);
 		if (err < 0)
-			return;
+			return err;
 
 		here = start + offset + 3;
 		end = start + offset + err;

@@ -8181,9 +8189,12 @@ static void niu_pci_vpd_fetch(struct niu *np, u32 start)
 		offset += err;
 
 		err = niu_pci_vpd_scan_props(np, here, end);
-		if (err < 0 || err == 1)
-			return;
+		if (err < 0)
+			return err;
+		if (err == 1)
+			return -EINVAL;
 	}
+	return 0;
 }
 
 /* ESPC_PIO_EN_ENABLE must be set */

@@ -9274,8 +9285,11 @@ static int niu_get_invariants(struct niu *np)
 		offset = niu_pci_vpd_offset(np);
 		netif_printk(np, probe, KERN_DEBUG, np->dev,
 			     "%s() VPD offset [%08x]\n", __func__, offset);
-		if (offset)
-			niu_pci_vpd_fetch(np, offset);
+		if (offset) {
+			err = niu_pci_vpd_fetch(np, offset);
+			if (err < 0)
+				return err;
+		}
 		nw64(ESPC_PIO_EN, 0);
 
 		if (np->flags & NIU_FLAGS_VPD_VALID) {

2.31.1

This reverts commit 5bf7295fe34a5251b1d241b9736af4697b590670.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

This commit does not properly detect if an error happens because the logic after this loop will not detect that there was a failed allocation.

---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c | 2 --
 1 file changed, 2 deletions(-)

index d8a3ecaed3fc..985cf8cb2ec0 100644
@@ -1047,8 +1047,6 @@ int qlcnic_do_lb_test(struct qlcnic_adapter *adapter, u8 mode)
 
 	for (i = 0; i < QLCNIC_NUM_ILB_PKT; i++) {
 		skb = netdev_alloc_skb(adapter->netdev, QLCNIC_ILB_PKT_SIZE);
-		if (!skb)
-			break;
 		qlcnic_create_loopback_buff(skb->data, adapter->mac_addr);
 		skb_put(skb, QLCNIC_ILB_PKT_SIZE);
 		adapter->ahw->diag_cnt = 0;

2.31.1

From: Tom Seewald <[email protected]>

The function qlcnic_dl_lb_test() currently calls netdev_alloc_skb() without checking afterwards that the allocation succeeded. Fix this by checking if the skb is NULL and returning an error in such a case. Breaking out of the loop if the skb is NULL is not correct as no error would be reported to the caller and no message would be printed for the user.

---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c | 3 +++
 1 file changed, 3 insertions(+)

index 985cf8cb2ec0..d8f0863b3934 100644
@@ -1047,6 +1047,8 @@ int qlcnic_do_lb_test(struct qlcnic_adapter *adapter, u8 mode)
 
 	for (i = 0; i < QLCNIC_NUM_ILB_PKT; i++) {
 		skb = netdev_alloc_skb(adapter->netdev, QLCNIC_ILB_PKT_SIZE);
+		if (!skb)
+			goto error;
 		qlcnic_create_loopback_buff(skb->data, adapter->mac_addr);
 		skb_put(skb, QLCNIC_ILB_PKT_SIZE);
 		adapter->ahw->diag_cnt = 0;

@@ -1070,6 +1072,7 @@ int qlcnic_do_lb_test(struct qlcnic_adapter *adapter, u8 mode)
 			cnt++;
 	}
 	if (cnt != i) {
+error:
 		dev_err(&adapter->pdev->dev,
 			"LB Test: failed, TX[%d], RX[%d]\n", i, cnt);
 		if (mode != QLCNIC_ILB_MODE)

2.31.1

This reverts commit 093c48213ee37c3c3ff1cf5ac1aa2a9d8bc66017.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

Because of this, all submissions from this group must be reverted from the kernel tree and will need to be re-reviewed again to determine if they actually are a valid fix. Until that work is complete, remove this change to ensure that no problems are being introduced into the codebase.

---
 drivers/cdrom/gdrom.c | 1 -
 1 file changed, 1 deletion(-)

index 742b4a0932e3..7f681320c7d3 100644
@@ -862,7 +862,6 @@ static void __exit exit_gdrom(void)
 	platform_device_unregister(pd);
 	platform_driver_unregister(&gdrom_driver);
 	kfree(gd.toc);
-	kfree(gd.cd_info);
 }
 
 module_init(init_gdrom);

2.31.1

From: Atul Gopinathan <[email protected]>

The fields, "toc" and "cd_info", of "struct gdrom_unit gd" are allocated in "probe_gdrom()". Prevent a memory leak by making sure "gd.cd_info" is deallocated in the "remove_gdrom()" function.

Also prevent double free of the field "gd.toc" by moving it from the module's exit function to "remove_gdrom()". This is because, in "probe_gdrom()", the function makes sure to deallocate "gd.toc" in case of any errors, so the exit function invoked later would again free "gd.toc".

The patch also maintains consistency by deallocating the above mentioned fields in "remove_gdrom()" along with another memory allocated field "gd.disk".

---
 drivers/cdrom/gdrom.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 7f681320c7d3..6c4f6139f853 100644
@@ -830,6 +830,8 @@ static int remove_gdrom(struct platform_device *devptr)
 	if (gdrom_major)
 		unregister_blkdev(gdrom_major, GDROM_DEV_NAME);
 	unregister_cdrom(gd.cd_info);
+	kfree(gd.cd_info);
+	kfree(gd.toc);
 
 	return 0;
 }

@@ -861,7 +863,6 @@ static void __exit exit_gdrom(void)
 {
 	platform_device_unregister(pd);
 	platform_driver_unregister(&gdrom_driver);
-	kfree(gd.toc);
 }
 
 module_init(init_gdrom);

2.31.1

Hi!

	memset(&gd, 0, sizeof(gd));

Yeah, that's bogus.

But can that really happen given that it hasn't ever happened before in a real system? :)

So should we set it to NULL after freeing it? Is that really going to help here given that the probe failed? Nothing can use it after remove_gdrom() is called because unregiser_* is called already.

I don't see the race here, sorry.

Ok, but again, how can that be used after remove_gdrom() is called?

Really, that's what is happening today as there is only 1 device here, and the whole structure was zeroed out already. So that would be a no-op.

Yes, ideally that would be the correct change, but given that you can only have 1 device in the system at a time of this type, it's not going to make much difference at all here.

Why? It fixes the obvious memory leak, right? Worst case you are saying we should also set to NULL these pointers, but I can not see how they are accessed as we have already torn everything down.

thanks,

greg k-h

Hi!

I'm thinking this:

1. init_gdrom() is called. gd.toc is NULL and is bogusly re-set to NULL.
2. probe_gdrom() is called and succeeds. gd.toc is allocted.
3. device is used, etc etc, whatever
4. remove_gdrom() is called. gd.toc is freed (but not set to NULL).
5. probe_gdrom() is called again. Boom.

In 5, gd.toc is not NULL, and is pointing to whatever. It is potentially used by probe_gdrom() before it is (re-)allocated.

I suppose the above can only happen if the module is compiled in.

Without this patch, we are "safe" because gd.toc still points to the old thing which is leaked once a new gd.toc is allocated by the second probe.

Cheers,
Peter

Ah. Well, adding/removing platform devices is a hard thing, and if you do it, you deserve the pieces you get :)

It would be trivial to fix this by setting all of &gd to 0 as you mention above, so yes, that would be good. But that's an add-on patch and not relevant to this "fix" here.

You can add/remove platform devices through sysfs if the code is a module as well.

I'll go make a new commit that zeros everything at probe_gdrom() that goes on top of this one.

thanks,

greg k-h

From: Greg Kroah-Hartman <[email protected]>

As Peter points out, if we were to disconnect and then reconnect this driver from a device, the "global" state of the device would contain odd values and could cause problems. Fix this up by just initializing the whole thing to 0 at probe() time.

Ideally this would be a per-device variable, but given the age and the total lack of users of it, that would require a lot of s/./->/g changes for really no good reason.

---

Note, this goes on top of my previous "gdrom" patch submitted here:

	https://lore.kernel.org/lkml/[email protected]/

And I'll just take it in the same series.

 drivers/cdrom/gdrom.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index 6c4f6139f853..c6d8c0f59722 100644
@@ -744,6 +744,13 @@ static const struct blk_mq_ops gdrom_mq_ops = {
 static int probe_gdrom(struct platform_device *devptr)
 {
 	int err;
+
+	/*
+	 * Ensure our "one" device is initialized properly in case of previous
+	 * usages of it
+	 */
+	memset(&gd, 0, sizeof(gd));
+
 	/* Start the device */
 	if (gdrom_execute_diagnostic() != 1) {
 		pr_warn("ATA Probe for GDROM failed\n");

@@ -847,7 +854,7 @@ static struct platform_driver gdrom_driver = {
 static int __init init_gdrom(void)
 {
 	int rc;
-	gd.toc = NULL;
+
 	rc = platform_driver_register(&gdrom_driver);
 	if (rc)
 		return rc;

2.31.1

Hi!

Looks good to me.

Thanks,
Peter

I guess I'm late and it seems like a conclusion has already been reached, so this mail doesn't really add up to anything. I just had a doubt in my mind which I wanted to clarify:

as Peter said, probe_gdrom() calls "probe_gdrom_setupcd()" which defines the ops, this includes "gdrom_get_last_session()" which is the only function that uses the data of "gd.toc".

It then calls "register_cdrom()", I went through the function definition of this and found only one line which has anything to do with ".get_last_session":

	int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi)
	{
		static char banner_printed;
		const struct cdrom_device_ops *cdo = cdi->ops;
		.
		.<snipped>
		.
----->		ENSURE(cdo, get_last_session, CDC_MULTI_SESSION);
		.
	}

The defintion of the ENSURE macro is this:

	#define ENSURE(cdo, call, bits)					\
	do {								\
		if (cdo->call == NULL)					\
			WARN_ON_ONCE((cdo)->capability & (bits));	\
	} while (0)

So here it is only checking if .get_last_session field is null or not, and not calling it.

Apart from this, I don't see gdrom_get_last_session() being called anywhere. But I could be missing something obvious too.

If you don't mind, could you point out where gd.toc is being used in probe_gdrom() before it is kzalloc-ed in the same function.

Thanks for the review!
Atul

Hi!

You are very probably correct in your analysis, and I can't find it in me to spend the time to dig any further.

I simply thought it bad enough to hand off a pointer to a function that uses a stale pointer to some other driver. I never dug into that other module like you did. Relying on that other piece of code to not use the function that was just handed to it is way too subtle (for me at least). When you "register" with something else, you should be ready to get the calls.

This is true especially in the context of what we are fixing up here; broken shit related to people that are fond of weaknesses later to be activated by other innocuous commits.

Cheers,
Peter

Ah, I see, that makes sense. I just wanted to confirm if I was getting things right. Thanks for clarifying!

Regards,
Atul

This reverts commit 13bd14a41ce3105d5b1f3cd8b4d1e249d17b6d9b.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

While this is technically correct, it is only fixing ONE of these errors in this function, so the patch is not fully correct. I'll leave this revert and provide a fix for this later that resolves this same "problem" everywhere in this function.

---
 drivers/char/hpet.c | 2 --
 1 file changed, 2 deletions(-)

index ed3b7dab678d..6f13def6c172 100644
@@ -969,8 +969,6 @@ static acpi_status hpet_resources(struct acpi_resource *res, void *data)
 	if (ACPI_SUCCESS(status)) {
 		hdp->hd_phys_address = addr.address.minimum;
 		hdp->hd_address = ioremap(addr.address.minimum, addr.address.address_length);
-		if (!hdp->hd_address)
-			return AE_ERROR;
 
 		if (hpet_is_known(hdp)) {
 			iounmap(hdp->hd_address);

2.31.1

From: Tom Seewald <[email protected]>

The function hpet_resources() calls ioremap() two times, but in both cases it does not check if ioremap() returned a null pointer. Fix this by adding null pointer checks and returning an appropriate error.

---
 drivers/char/hpet.c | 4 ++++
 1 file changed, 4 insertions(+)

index 6f13def6c172..8b55085650ad 100644
@@ -969,6 +969,8 @@ static acpi_status hpet_resources(struct acpi_resource *res, void *data)
 	if (ACPI_SUCCESS(status)) {
 		hdp->hd_phys_address = addr.address.minimum;
 		hdp->hd_address = ioremap(addr.address.minimum, addr.address.address_length);
+		if (!hdp->hd_address)
+			return AE_ERROR;
 
 		if (hpet_is_known(hdp)) {
 			iounmap(hdp->hd_address);

@@ -982,6 +984,8 @@ static acpi_status hpet_resources(struct acpi_resource *res, void *data)
 		hdp->hd_phys_address = fixmem32->address;
 		hdp->hd_address = ioremap(fixmem32->address,
 						HPET_RANGE_SIZE);
+		if (!hdp->hd_address)
+			return AE_ERROR;
 
 		if (hpet_is_known(hdp)) {
 			iounmap(hdp->hd_address);

2.31.1

This reverts commit 63a06181d7ce169d09843645c50fea1901bc9f0a.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original commit is incorrect, it does not properly clean up on the error path, so I'll keep the revert and fix it up properly with a follow-on patch.

---
 drivers/scsi/ufs/ufs-hisi.c | 4 ----
 1 file changed, 4 deletions(-)

index 0aa58131e791..7d1e07a9d9dd 100644
@@ -468,10 +468,6 @@ static int ufs_hisi_init_common(struct ufs_hba *hba)
 	ufshcd_set_variant(hba, host);
 
 	host->rst  = devm_reset_control_get(dev, "rst");
-	if (IS_ERR(host->rst)) {
-		dev_err(dev, "%s: failed to get reset control\n", __func__);
-		return PTR_ERR(host->rst);
-	}
 
 	ufs_hisi_set_pm_lvl(hba);
 

2.31.1

From: Phillip Potter <[email protected]>

Move ufshcd_set_variant call in ufs_hisi_init_common to common error section at end of the function, and then jump to this from the error checking statements for both devm_reset_control_get and ufs_hisi_get_resource. This fixes the original commit (63a06181d7ce) which was reverted due to the University of Minnesota problems.

---
 drivers/scsi/ufs/ufs-hisi.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

index 7d1e07a9d9dd..d0626773eb38 100644
@@ -467,17 +467,24 @@ static int ufs_hisi_init_common(struct ufs_hba *hba)
 	host->hba = hba;
 	ufshcd_set_variant(hba, host);
 
-	host->rst  = devm_reset_control_get(dev, "rst");
+	host->rst = devm_reset_control_get(dev, "rst");
+	if (IS_ERR(host->rst)) {
+		dev_err(dev, "%s: failed to get reset control\n", __func__);
+		err = PTR_ERR(host->rst);
+		goto error;
+	}
 
 	ufs_hisi_set_pm_lvl(hba);
 
 	err = ufs_hisi_get_resource(host);
-	if (err) {
-		ufshcd_set_variant(hba, NULL);
-		return err;
-	}
+	if (err)
+		goto error;
 
 	return 0;
+
+error:
+	ufshcd_set_variant(hba, NULL);
+	return err;
 }
 
 static int ufs_hi3660_init(struct ufs_hba *hba)

2.31.1

This reverts commit 0f25e000cb4398081748e54f62a902098aa79ec1.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original commit did nothing if there was an error, except to print out a message, which is pointless. So remove the commit as it gives a "false sense of doing something".

---
 sound/isa/gus/gus_main.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

index afc088f0377c..b7518122a10d 100644
@@ -77,17 +77,8 @@ static const struct snd_kcontrol_new snd_gus_joystick_control = {
 
 static void snd_gus_init_control(struct snd_gus_card *gus)
 {
-	int ret;
-
-	if (!gus->ace_flag) {
-		ret =
-			snd_ctl_add(gus->card,
-					snd_ctl_new1(&snd_gus_joystick_control,
-						gus));
-		if (ret)
-			snd_printk(KERN_ERR "gus: snd_ctl_add failed: %d\n",
-					ret);
-	}
+	if (!gus->ace_flag)
+		snd_ctl_add(gus->card, snd_ctl_new1(&snd_gus_joystick_control, gus));
 }
 
 /*

2.31.1

From: Kurt Manucredo <[email protected]>

snd_gus_init_control() does not properly return any possible error that might have happened in a call to snd_ctl_add() so resolve this by propagating the error back up the call change correctly.

---
 sound/isa/gus/gus_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index b7518122a10d..4c2703ea55fb 100644
@@ -75,10 +75,11 @@ static const struct snd_kcontrol_new snd_gus_joystick_control = {
 	.put = snd_gus_joystick_put
 };
 
-static void snd_gus_init_control(struct snd_gus_card *gus)
+static int snd_gus_init_control(struct snd_gus_card *gus)
 {
 	if (!gus->ace_flag)
-		snd_ctl_add(gus->card, snd_ctl_new1(&snd_gus_joystick_control, gus));
+		return snd_ctl_add(gus->card, snd_ctl_new1(&snd_gus_joystick_control, gus));
+	return 0;
 }
 
 /*

@@ -386,8 +387,7 @@ static int snd_gus_check_version(struct snd_gus_card * gus)
 	}
 	strcpy(card->shortname, card->longname);
 	gus->uart_enable = 1;	/* standard GUSes doesn't have midi uart trouble */
-	snd_gus_init_control(gus);
-	return 0;
+	return snd_gus_init_control(gus);
 }
 
 int snd_gus_initialize(struct snd_gus_card *gus)

2.31.1

This change doesn't look good, either.
It results in the bogus error message "version check failed".

If we really want to fix this, it's better to call snd_gus_init_control() from snd_gus_initialize() itself while changing snd_gus_init_control() to return an error code.

However, this error is really not what we should be bothered too much. Even if a creation of control element failed, it's no fatal error and nothing really wrong would happen by itself. And, under such a situation, the system memory is already too tight and the OS would hang up sooner or later (or OOM killer starts genocide).

thanks,

Takashi

Thanks for the review, I'll just drop this change as it's not worth it for this type of "impossible to hit" issue.

thanks,

greg k-h

This reverts commit dcd0feac9bab901d5739de51b3f69840851f8919.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original commit message for this change was incorrect as the code path can never result in a NULL dereference, alluding to the fact that whatever tool was used to "find this" is broken. It's just an optional resource reservation, so removing this check is fine.

---
 sound/isa/sb/sb8.c | 4 ----
 1 file changed, 4 deletions(-)

index 6c9d534ce8b6..95290ffe5c6e 100644
@@ -95,10 +95,6 @@ static int snd_sb8_probe(struct device *pdev, unsigned int dev)
 
 	/* block the 0x388 port to avoid PnP conflicts */
 	acard->fm_res = request_region(0x388, 4, "SoundBlaster FM");
-	if (!acard->fm_res) {
-		err = -EBUSY;
-		goto _err;
-	}
 
 	if (port[dev] != SNDRV_AUTO_PORT) {
 		if ((err = snd_sbdsp_create(card, port[dev], irq[dev],

2.31.1

From: Atul Gopinathan <[email protected]>

The field "fm_res" of "struct snd_sb8" is never used/dereferenced throughout the sb8.c code. Therefore there is no need for any null value check after the "request_region()".

Add a comment note to make developers know about this and prevent any "NULL check" patches on this part of code.

---
 sound/isa/sb/sb8.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 95290ffe5c6e..ed3a87ebe3f4 100644
@@ -93,7 +93,11 @@ static int snd_sb8_probe(struct device *pdev, unsigned int dev)
 	acard = card->private_data;
 	card->private_free = snd_sb8_free;
 
-	/* block the 0x388 port to avoid PnP conflicts */
+	/*
+	 * Block the 0x388 port to avoid PnP conflicts.
+	 * No need to check this value after request_region,
+	 * as we never do anything with it.
+	 */
 	acard->fm_res = request_region(0x388, 4, "SoundBlaster FM");
 
 	if (port[dev] != SNDRV_AUTO_PORT) {

2.31.1

This reverts commit a2c6433ee5a35a8de6d563f6512a26f87835ea0f.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original patch was incorrect, and would leak memory if the error path the patch added was hit.

---
 sound/usb/usx2y/usb_stream.c | 5 -----
 1 file changed, 5 deletions(-)

index 091c071b270a..6bba17bf689a 100644
@@ -91,12 +91,7 @@ static int init_urbs(struct usb_stream_kernel *sk, unsigned use_packsize,
 
 	for (u = 0; u < USB_STREAM_NURBS; ++u) {
 		sk->inurb[u] = usb_alloc_urb(sk->n_o_ps, GFP_KERNEL);
-		if (!sk->inurb[u])
-			return -ENOMEM;
-
 		sk->outurb[u] = usb_alloc_urb(sk->n_o_ps, GFP_KERNEL);
-		if (!sk->outurb[u])
-			return -ENOMEM;
 	}
 
 	if (init_pipe_urbs(sk, use_packsize, sk->inurb, indata, dev, in_pipe) ||

2.31.1

While it is almost impossible to hit an error calling usb_alloc_urb(), to make systems like syzbot which does error injection, and some static analysis tools happy, properly handle errors on this path by unwinding the previously allocated urbs and freeing them.

---
 sound/usb/usx2y/usb_stream.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

index 6bba17bf689a..1091ea96a29a 100644
@@ -88,18 +88,35 @@ static int init_urbs(struct usb_stream_kernel *sk, unsigned use_packsize,
 					sizeof(struct usb_stream_packet) *
 					s->inpackets;
 	int			u;
+	int			i;
+	int			err = -ENOMEM;
 
 	for (u = 0; u < USB_STREAM_NURBS; ++u) {
+		sk->outurb[u] = NULL;
 		sk->inurb[u] = usb_alloc_urb(sk->n_o_ps, GFP_KERNEL);
+		if (!sk->inurb[u])
+			goto error;
 		sk->outurb[u] = usb_alloc_urb(sk->n_o_ps, GFP_KERNEL);
+		if (!sk->outurb[u])
+			goto error;
 	}
+	u--;
 
 	if (init_pipe_urbs(sk, use_packsize, sk->inurb, indata, dev, in_pipe) ||
 	    init_pipe_urbs(sk, use_packsize, sk->outurb, sk->write_page, dev,
-			   out_pipe))
-		return -EINVAL;
+			   out_pipe)) {
+		err = -EINVAL;
+		goto error;
+	}
 
 	return 0;
+
+error:
+	for (i = 0; i <= u; ++i) {
+		usb_free_urb(sk->inurb[i]);
+		usb_free_urb(sk->outurb[i]);
+	}
+	return err;
 }
 
 

2.31.1

Dne 03. 05. 21 v 13:57 Greg Kroah-Hartman napsal(a):

Perhaps, I miss something, but this revert and add the cleanup to the wrong place makes things worse:

The usb_stream_free() is called when init_urbs() fails (returns an error), so all urbs are freed there and pointers are reset to NULL. Your code frees urbs twice on an allocation error.

The reverted code does the job better.

Jaroslav

Jaroslav Kysela <[email protected]>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Right, the suggested patch will cause the double-free, and the reverted code is fine in this regard.

In anyway, the cleanup of this driver code is on my post-15.3 TODO list. So, Greg, could you drop the revert and the additional fix at this round for usx2y driver?

Thanks!

Takashi

That wasn't obvious at all, sorry about that.

Ok, will drop the revert and this patch, thanks for the review and sorry for the noise.

greg k-h

This reverts commit ec7f6aad57ad29e4e66cc2e18e1e1599ddb02542.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

This patch "looks" correct, but the driver keeps on running and will fail horribly right afterward if this error condition ever trips.

So points for trying to resolve an issue, but a huge NEGATIVE value for providing a "fake" fix for the problem as nothing actually got resolved at all. I'll go fix this up properly...

---
 drivers/video/fbdev/hgafb.c | 2 --
 1 file changed, 2 deletions(-)

index 8bbac7182ad3..fca29f219f8b 100644
@@ -285,8 +285,6 @@ static int hga_card_detect(void)
 	hga_vram_len  = 0x08000;
 
 	hga_vram = ioremap(0xb0000, hga_vram_len);
-	if (!hga_vram)
-		goto error;
 
 	if (request_region(0x3b0, 12, "hgafb"))
 		release_io_ports = 1;

2.31.1

From: Igor Matheus Andrade Torrente <[email protected]>

The return of ioremap if not checked, and can lead to a NULL to be assigned to hga_vram. Potentially leading to a NULL pointer dereference.

The fix adds code to deal with this case in the error label and changes how the hgafb_probe handles the return of hga_card_detect.

---
 drivers/video/fbdev/hgafb.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

index fca29f219f8b..cc8e62ae93f6 100644
@@ -285,6 +285,8 @@ static int hga_card_detect(void)
 	hga_vram_len  = 0x08000;
 
 	hga_vram = ioremap(0xb0000, hga_vram_len);
+	if (!hga_vram)
+		return -ENOMEM;
 
 	if (request_region(0x3b0, 12, "hgafb"))
 		release_io_ports = 1;

@@ -344,13 +346,18 @@ static int hga_card_detect(void)
 			hga_type_name = "Hercules";
 			break;
 	}
-	return 1;
+	return 0;
 error:
 	if (release_io_ports)
 		release_region(0x3b0, 12);
 	if (release_io_port)
 		release_region(0x3bf, 1);
-	return 0;
+
+	iounmap(hga_vram);
+
+	pr_err("hgafb: HGA card not detected.\n");
+
+	return -EINVAL;
 }
 
 /**

@@ -548,13 +555,11 @@ static const struct fb_ops hgafb_ops = {
 static int hgafb_probe(struct platform_device *pdev)
 {
 	struct fb_info *info;
+	int ret;
 
-	if (! hga_card_detect()) {
-		printk(KERN_INFO "hgafb: HGA card not detected.\n");
-		if (hga_vram)
-			iounmap(hga_vram);
-		return -EINVAL;
-	}
+	ret = hga_card_detect();
+	if (!ret)
+		return ret;
 
 	printk(KERN_INFO "hgafb: %s with %ldK of memory detected.\n",
 		hga_type_name, hga_vram_len/1024);

2.31.1

This reverts commit d721fe99f6ada070ae8fc0ec3e01ce5a42def0d9.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original commit was incorrect, it should have never have used "unlikely()" and if it ever does trigger, resources are left grabbed.

Given there are no users for this code around, I'll just revert this and leave it "as is" as the odds that ioremap() will ever fail here is horrendiously low.

---
 drivers/isdn/hardware/mISDN/mISDNinfineon.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index a16c7a2a7f3d..fa9c491f9c38 100644
@@ -697,11 +697,8 @@ setup_io(struct inf_hw *hw)
 				(ulong)hw->addr.start, (ulong)hw->addr.size);
 			return err;
 		}
-		if (hw->ci->addr_mode == AM_MEMIO) {
+		if (hw->ci->addr_mode == AM_MEMIO)
 			hw->addr.p = ioremap(hw->addr.start, hw->addr.size);
-			if (unlikely(!hw->addr.p))
-				return -ENOMEM;
-		}
 		hw->addr.mode = hw->ci->addr_mode;
 		if (debug & DEBUG_HW)
 			pr_notice("%s: IO addr %lx (%lu bytes) mode%d\n",

2.31.1

From: Phillip Potter <[email protected]>

Move hw->cfg.mode and hw->addr.mode assignments from hw->ci->cfg_mode and hw->ci->addr_mode respectively, to be before the subsequent checks for memory IO mode (and possible ioremap calls in this case).

Also introduce ioremap error checks at both locations. This allows resources to be properly freed on ioremap failure, as when the caller of setup_io then subsequently calls release_io via its error path, release_io can now correctly determine the mode as it has been set before the ioremap call.

Finally, refactor release_io function so that it will call release_mem_region in the memory IO case, regardless of whether or not hw->cfg.p/hw->addr.p are NULL. This means resources are then properly released on failure.

This properly implements the original reverted commit (d721fe99f6ad) from the University of Minnesota, whilst also implementing the ioremap check for the hw->ci->cfg_mode if block as well.

---
 drivers/isdn/hardware/mISDN/mISDNinfineon.c | 24 ++++++++++++++-------
 1 file changed, 16 insertions(+), 8 deletions(-)

index fa9c491f9c38..88d592bafdb0 100644
@@ -630,17 +630,19 @@ static void
 release_io(struct inf_hw *hw)
 {
 	if (hw->cfg.mode) {
-		if (hw->cfg.p) {
+		if (hw->cfg.mode == AM_MEMIO) {
 			release_mem_region(hw->cfg.start, hw->cfg.size);
-			iounmap(hw->cfg.p);
+			if (hw->cfg.p)
+				iounmap(hw->cfg.p);
 		} else
 			release_region(hw->cfg.start, hw->cfg.size);
 		hw->cfg.mode = AM_NONE;
 	}
 	if (hw->addr.mode) {
-		if (hw->addr.p) {
+		if (hw->addr.mode == AM_MEMIO) {
 			release_mem_region(hw->addr.start, hw->addr.size);
-			iounmap(hw->addr.p);
+			if (hw->addr.p)
+				iounmap(hw->addr.p);
 		} else
 			release_region(hw->addr.start, hw->addr.size);
 		hw->addr.mode = AM_NONE;

@@ -670,9 +672,12 @@ setup_io(struct inf_hw *hw)
 				(ulong)hw->cfg.start, (ulong)hw->cfg.size);
 			return err;
 		}
-		if (hw->ci->cfg_mode == AM_MEMIO)
-			hw->cfg.p = ioremap(hw->cfg.start, hw->cfg.size);
 		hw->cfg.mode = hw->ci->cfg_mode;
+		if (hw->ci->cfg_mode == AM_MEMIO) {
+			hw->cfg.p = ioremap(hw->cfg.start, hw->cfg.size);
+			if (!hw->cfg.p)
+				return -ENOMEM;
+		}
 		if (debug & DEBUG_HW)
 			pr_notice("%s: IO cfg %lx (%lu bytes) mode%d\n",
 				  hw->name, (ulong)hw->cfg.start,

@@ -697,9 +702,12 @@ setup_io(struct inf_hw *hw)
 				(ulong)hw->addr.start, (ulong)hw->addr.size);
 			return err;
 		}
-		if (hw->ci->addr_mode == AM_MEMIO)
-			hw->addr.p = ioremap(hw->addr.start, hw->addr.size);
 		hw->addr.mode = hw->ci->addr_mode;
+		if (hw->ci->addr_mode == AM_MEMIO) {
+			hw->addr.p = ioremap(hw->addr.start, hw->addr.size);
+			if (!hw->addr.p)
+				return -ENOMEM;
+		}
 		if (debug & DEBUG_HW)
 			pr_notice("%s: IO addr %lx (%lu bytes) mode%d\n",
 				  hw->name, (ulong)hw->addr.start,

2.31.1

This reverts commit fc6a6521556c8250e356ddc6a3f2391aa62dc976.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The change being reverted does NOTHING as the caller to this function does not even look at the return value of the call. So the "claim" that this fixed an an issue is not true. It will be fixed up properly in a future patch by propagating the error up the stack correctly.

---
 drivers/net/wireless/ath/ath6kl/wmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index b137e7f34397..aca9732ec1ee 100644
@@ -776,8 +776,10 @@ int ath6kl_wmi_set_roam_lrssi_cmd(struct wmi *wmi, u8 lrssi)
 	cmd->info.params.roam_rssi_floor = DEF_LRSSI_ROAM_FLOOR;
 	cmd->roam_ctrl = WMI_SET_LRSSI_SCAN_PARAMS;
 
-	return ath6kl_wmi_cmd_send(wmi, 0, skb, WMI_SET_ROAM_CTRL_CMDID,
+	ath6kl_wmi_cmd_send(wmi, 0, skb, WMI_SET_ROAM_CTRL_CMDID,
 			    NO_SYNC_WMIFLAG);
+
+	return 0;
 }
 
 int ath6kl_wmi_force_roam_cmd(struct wmi *wmi, const u8 *bssid)

2.31.1

From: Anirudh Rayabharam <[email protected]>

Propagate error code from failure of ath6kl_wmi_cmd_send() to the caller.

---
 drivers/net/wireless/ath/ath6kl/debug.c | 5 ++++-
 drivers/net/wireless/ath/ath6kl/wmi.c   | 4 +---
 2 files changed, 5 insertions(+), 4 deletions(-)

index 7506cea46f58..433a047f3747 100644
@@ -1027,14 +1027,17 @@ static ssize_t ath6kl_lrssi_roam_write(struct file *file,
 {
 	struct ath6kl *ar = file->private_data;
 	unsigned long lrssi_roam_threshold;
+	int ret;
 
 	if (kstrtoul_from_user(user_buf, count, 0, &lrssi_roam_threshold))
 		return -EINVAL;
 
 	ar->lrssi_roam_threshold = lrssi_roam_threshold;
 
-	ath6kl_wmi_set_roam_lrssi_cmd(ar->wmi, ar->lrssi_roam_threshold);
+	ret = ath6kl_wmi_set_roam_lrssi_cmd(ar->wmi, ar->lrssi_roam_threshold);
 
+	if (ret)
+		return ret;
 	return count;
 }
 

index aca9732ec1ee..b137e7f34397 100644
@@ -776,10 +776,8 @@ int ath6kl_wmi_set_roam_lrssi_cmd(struct wmi *wmi, u8 lrssi)
 	cmd->info.params.roam_rssi_floor = DEF_LRSSI_ROAM_FLOOR;
 	cmd->roam_ctrl = WMI_SET_LRSSI_SCAN_PARAMS;
 
-	ath6kl_wmi_cmd_send(wmi, 0, skb, WMI_SET_ROAM_CTRL_CMDID,
+	return ath6kl_wmi_cmd_send(wmi, 0, skb, WMI_SET_ROAM_CTRL_CMDID,
 			    NO_SYNC_WMIFLAG);
-
-	return 0;
 }
 
 int ath6kl_wmi_force_roam_cmd(struct wmi *wmi, const u8 *bssid)

2.31.1

This reverts commit 23015b22e47c5409620b1726a677d69e5cd032ba.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original commit has a memory leak on the error path here, it does not clean up everything properly.

---
 drivers/rapidio/rio_cm.c | 8 --------
 1 file changed, 8 deletions(-)

index 50ec53d67a4c..e6c16f04f2b4 100644
@@ -2138,14 +2138,6 @@ static int riocm_add_mport(struct device *dev,
 	mutex_init(&cm->rx_lock);
 	riocm_rx_fill(cm, RIOCM_RX_RING_SIZE);
 	cm->rx_wq = create_workqueue(DRV_NAME "/rxq");
-	if (!cm->rx_wq) {
-		riocm_error("failed to allocate IBMBOX_%d on %s",
-			    cmbox, mport->name);
-		rio_release_outb_mbox(mport, cmbox);
-		kfree(cm);
-		return -ENOMEM;
-	}
-
 	INIT_WORK(&cm->rx_work, rio_ibmsg_handler);
 
 	cm->tx_slot = 0;

2.31.1

From: Anirudh Rayabharam <[email protected]>

In case create_workqueue() fails, release all resources and return -ENOMEM to caller to avoid potential NULL pointer deref later. Move up the create_workequeue() call to return early and avoid unwinding the call to riocm_rx_fill().

---
 drivers/rapidio/rio_cm.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index e6c16f04f2b4..db4c265287ae 100644
@@ -2127,6 +2127,14 @@ static int riocm_add_mport(struct device *dev,
 		return -ENODEV;
 	}
 
+	cm->rx_wq = create_workqueue(DRV_NAME "/rxq");
+	if (!cm->rx_wq) {
+		rio_release_inb_mbox(mport, cmbox);
+		rio_release_outb_mbox(mport, cmbox);
+		kfree(cm);
+		return -ENOMEM;
+	}
+
 	/*
 	 * Allocate and register inbound messaging buffers to be ready
 	 * to receive channel and system management requests

@@ -2137,7 +2145,6 @@ static int riocm_add_mport(struct device *dev,
 	cm->rx_slots = RIOCM_RX_RING_SIZE;
 	mutex_init(&cm->rx_lock);
 	riocm_rx_fill(cm, RIOCM_RX_RING_SIZE);
-	cm->rx_wq = create_workqueue(DRV_NAME "/rxq");
 	INIT_WORK(&cm->rx_work, rio_ibmsg_handler);
 
 	cm->tx_slot = 0;

2.31.1

This reverts commit 38d22659803a033b1b66cd2624c33570c0dde77d.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

While it looks like the original change is correct, it is not, as none of the setup actually happens, and the error value is not propagated upwards.

---
 drivers/isdn/hardware/mISDN/hfcsusb.c | 3 ---
 1 file changed, 3 deletions(-)

index 70061991915a..4bb470d3963d 100644
@@ -249,9 +249,6 @@ hfcsusb_ph_info(struct hfcsusb *hw)
 	int i;
 
 	phi = kzalloc(struct_size(phi, bch, dch->dev.nrbchan), GFP_ATOMIC);
-	if (!phi)
-		return;
-
 	phi->dch.ch.protocol = hw->protocol;
 	phi->dch.ch.Flags = dch->Flags;
 	phi->dch.state = dch->state;

2.31.1

From: Phillip Potter <[email protected]>

Modify return type of hfcusb_ph_info to int, so that we can pass error value up the call stack when allocation of ph_info fails. Also change three of four call sites to actually account for the memory failure. The fourth, in ph_state_nt, is infeasible to change as it is in turn called by ph_state which is used as a function pointer argument to mISDN_initdchannel, which would necessitate changing its signature and updating all the places where it is used (too many).

Fixes original flawed commit (38d22659803a) from the University of Minnesota.

---
 drivers/isdn/hardware/mISDN/hfcsusb.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

index 4bb470d3963d..cd5642cef01f 100644
@@ -46,7 +46,7 @@ static void hfcsusb_start_endpoint(struct hfcsusb *hw, int channel);
 static void hfcsusb_stop_endpoint(struct hfcsusb *hw, int channel);
 static int  hfcsusb_setup_bch(struct bchannel *bch, int protocol);
 static void deactivate_bchannel(struct bchannel *bch);
-static void hfcsusb_ph_info(struct hfcsusb *hw);
+static int  hfcsusb_ph_info(struct hfcsusb *hw);
 
 /* start next background transfer for control channel */
 static void

@@ -241,7 +241,7 @@ hfcusb_l2l1B(struct mISDNchannel *ch, struct sk_buff *skb)
  * send full D/B channel status information
  * as MPH_INFORMATION_IND
  */
-static void
+static int
 hfcsusb_ph_info(struct hfcsusb *hw)
 {
 	struct ph_info *phi;

@@ -249,6 +249,9 @@ hfcsusb_ph_info(struct hfcsusb *hw)
 	int i;
 
 	phi = kzalloc(struct_size(phi, bch, dch->dev.nrbchan), GFP_ATOMIC);
+	if (!phi)
+		return -ENOMEM;
+
 	phi->dch.ch.protocol = hw->protocol;
 	phi->dch.ch.Flags = dch->Flags;
 	phi->dch.state = dch->state;

@@ -260,6 +263,8 @@ hfcsusb_ph_info(struct hfcsusb *hw)
 	_queue_data(&dch->dev.D, MPH_INFORMATION_IND, MISDN_ID_ANY,
 		    struct_size(phi, bch, dch->dev.nrbchan), phi, GFP_ATOMIC);
 	kfree(phi);
+
+	return 0;
 }
 
 /*

@@ -344,8 +349,7 @@ hfcusb_l2l1D(struct mISDNchannel *ch, struct sk_buff *skb)
 			ret = l1_event(dch->l1, hh->prim);
 		break;
 	case MPH_INFORMATION_REQ:
-		hfcsusb_ph_info(hw);
-		ret = 0;
+		ret = hfcsusb_ph_info(hw);
 		break;
 	}
 

@@ -400,8 +404,7 @@ hfc_l1callback(struct dchannel *dch, u_int cmd)
 			       hw->name, __func__, cmd);
 		return -1;
 	}
-	hfcsusb_ph_info(hw);
-	return 0;
+	return hfcsusb_ph_info(hw);
 }
 
 static int

@@ -743,8 +746,7 @@ hfcsusb_setup_bch(struct bchannel *bch, int protocol)
 			handle_led(hw, (bch->nr == 1) ? LED_B1_OFF :
 				   LED_B2_OFF);
 	}
-	hfcsusb_ph_info(hw);
-	return 0;
+	return hfcsusb_ph_info(hw);
 }
 
 static void

2.31.1

This reverts commit 2c2a7552dd6465e8fde6bc9cccf8d66ed1c1eb72.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original commit log for this change was incorrect, no "error handling code" was added, things will blow up just as badly as before if any of these cases ever were true. As this BUG_ON() never fired, and most of these checks are "obviously" never going to be true, let's just revert to the original code for now until this gets unwound to be done correctly in the future.

---
 fs/ecryptfs/crypto.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index 943e523f4c9d..3d8623139538 100644
@@ -296,10 +296,8 @@ static int crypt_scatterlist(struct ecryptfs_crypt_stat *crypt_stat,
 	struct extent_crypt_result ecr;
 	int rc = 0;
 
-	if (!crypt_stat || !crypt_stat->tfm
-	       || !(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED))
-		return -EINVAL;
-
+	BUG_ON(!crypt_stat || !crypt_stat->tfm
+	       || !(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED));
 	if (unlikely(ecryptfs_verbosity > 0)) {
 		ecryptfs_printk(KERN_DEBUG, "Key size [%zd]; key:\n",
 				crypt_stat->key_size);

2.31.1

From: Phillip Potter <[email protected]>

crypt_stat memory itself is allocated when inode is created, in ecryptfs_alloc_inode, which returns NULL on failure and is handled by callers, which would prevent us getting to this point. It then calls ecryptfs_init_crypt_stat which allocates crypt_stat->tfm checking for and likewise handling allocation failure. Finally, crypt_stat->flags has ECRYPTFS_STRUCT_INITIALIZED merged into it in ecryptfs_init_crypt_stat as well.

Simply put, the conditions that the BUG_ON checks for will never be triggered, as to even get to this function, the relevant conditions will have already been fulfilled (or the inode allocation would fail in the first place and thus no call to this function or those above it).

---
 fs/ecryptfs/crypto.c | 2 --
 1 file changed, 2 deletions(-)

index 3d8623139538..271064511c1b 100644
@@ -296,8 +296,6 @@ static int crypt_scatterlist(struct ecryptfs_crypt_stat *crypt_stat,
 	struct extent_crypt_result ecr;
 	int rc = 0;
 
-	BUG_ON(!crypt_stat || !crypt_stat->tfm
-	       || !(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED));
 	if (unlikely(ecryptfs_verbosity > 0)) {
 		ecryptfs_printk(KERN_DEBUG, "Key size [%zd]; key:\n",
 				crypt_stat->key_size);

2.31.1

This reverts commit a474b3f0428d6b02a538aa10b3c3b722751cb382.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original change is NOT correct, as it does not correctly unwind from the resources that was allocated before the call to platform_driver_register().

---
 drivers/dma/qcom/hidma_mgmt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 806ca02c52d7..fe87b01f7a4e 100644
@@ -418,8 +418,9 @@ static int __init hidma_mgmt_init(void)
 		hidma_mgmt_of_populate_channels(child);
 	}
 #endif
-	return platform_driver_register(&hidma_mgmt_driver);
+	platform_driver_register(&hidma_mgmt_driver);
 
+	return 0;
 }
 module_init(hidma_mgmt_init);
 MODULE_LICENSE("GPL v2");

2.31.1

From: Phillip Potter <[email protected]>

Place a comment in hidma_mgmt_init explaining why success must currently be assumed, due to the cleanup issue that would need to be considered were this module ever to be unloadable or were this platform_driver_register call ever to fail.

---
 drivers/dma/qcom/hidma_mgmt.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

index fe87b01f7a4e..62026607f3f8 100644
@@ -418,6 +418,20 @@ static int __init hidma_mgmt_init(void)
 		hidma_mgmt_of_populate_channels(child);
 	}
 #endif
+	/*
+	 * We do not check for return value here, as it is assumed that
+	 * platform_driver_register must not fail. The reason for this is that
+	 * the (potential) hidma_mgmt_of_populate_channels calls above are not
+	 * cleaned up if it does fail, and to do this work is quite
+	 * complicated. In particular, various calls of of_address_to_resource,
+	 * of_irq_to_resource, platform_device_register_full, of_dma_configure,
+	 * and of_msi_configure which then call other functions and so on, must
+	 * be cleaned up - this is not a trivial exercise.
+	 *
+	 * Currently, this module is not intended to be unloaded, and there is
+	 * no module_exit function defined which does the needed cleanup. For
+	 * this reason, we have to assume success here.
+	 */
 	platform_driver_register(&hidma_mgmt_driver);
 
 	return 0;

2.31.1

This reverts commit 434256833d8eb988cb7f3b8a41699e2fe48d9332.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

The original commit was incorrect, the error needs to be propagated back to the caller AND if the second group call fails, the first needs to be removed. There are much better ways to solve this, the driver should NOT be calling sysfs_create_group() on its own as it is racing userspace and loosing.

---
 drivers/net/wireless/marvell/libertas/mesh.c | 5 -----
 1 file changed, 5 deletions(-)

index f5b78257d551..c611e6668b21 100644
@@ -805,12 +805,7 @@ static void lbs_persist_config_init(struct net_device *dev)
 {
 	int ret;
 	ret = sysfs_create_group(&(dev->dev.kobj), &boot_opts_group);
-	if (ret)
-		pr_err("failed to create boot_opts_group.\n");
-
 	ret = sysfs_create_group(&(dev->dev.kobj), &mesh_ie_group);
-	if (ret)
-		pr_err("failed to create mesh_ie_group.\n");
 }
 
 static void lbs_persist_config_remove(struct net_device *dev)

2.31.1

The libertas driver was trying to register sysfs groups "by hand" which causes them to be created _after_ the device is initialized and announced to userspace, which causes races and can prevent userspace tools from seeing the sysfs files correctly.

Fix this up by using the built-in sysfs_groups pointers in struct net_device which were created for this very reason, fixing the race condition, and properly allowing for any error that might have occured to be handled properly.

---
 drivers/net/wireless/marvell/libertas/mesh.c | 28 +++-----------------
 1 file changed, 4 insertions(+), 24 deletions(-)

index c611e6668b21..c68814841583 100644
@@ -801,19 +801,6 @@ static const struct attribute_group mesh_ie_group = {
 	.attrs = mesh_ie_attrs,
 };
 
-static void lbs_persist_config_init(struct net_device *dev)
-{
-	int ret;
-	ret = sysfs_create_group(&(dev->dev.kobj), &boot_opts_group);
-	ret = sysfs_create_group(&(dev->dev.kobj), &mesh_ie_group);
-}
-
-static void lbs_persist_config_remove(struct net_device *dev)
-{
-	sysfs_remove_group(&(dev->dev.kobj), &boot_opts_group);
-	sysfs_remove_group(&(dev->dev.kobj), &mesh_ie_group);
-}
-
 
 /***************************************************************************
  * Initializing and starting, stopping mesh

@@ -1009,6 +996,10 @@ static int lbs_add_mesh(struct lbs_private *priv)
 	SET_NETDEV_DEV(priv->mesh_dev, priv->dev->dev.parent);
 
 	mesh_dev->flags |= IFF_BROADCAST | IFF_MULTICAST;
+	mesh_dev->sysfs_groups[0] = &lbs_mesh_attr_group;
+	mesh_dev->sysfs_groups[1] = &boot_opts_group;
+	mesh_dev->sysfs_groups[2] = &mesh_ie_group;
+
 	/* Register virtual mesh interface */
 	ret = register_netdev(mesh_dev);
 	if (ret) {

@@ -1016,19 +1007,10 @@ static int lbs_add_mesh(struct lbs_private *priv)
 		goto err_free_netdev;
 	}
 
-	ret = sysfs_create_group(&(mesh_dev->dev.kobj), &lbs_mesh_attr_group);
-	if (ret)
-		goto err_unregister;
-
-	lbs_persist_config_init(mesh_dev);
-
 	/* Everything successful */
 	ret = 0;
 	goto done;
 
-err_unregister:
-	unregister_netdev(mesh_dev);
-
 err_free_netdev:
 	free_netdev(mesh_dev);
 

@@ -1049,8 +1031,6 @@ void lbs_remove_mesh(struct lbs_private *priv)
 
 	netif_stop_queue(mesh_dev);
 	netif_carrier_off(mesh_dev);
-	sysfs_remove_group(&(mesh_dev->dev.kobj), &lbs_mesh_attr_group);
-	lbs_persist_config_remove(mesh_dev);
 	unregister_netdev(mesh_dev);
 	priv->mesh_dev = NULL;
 	kfree(mesh_dev->ieee80211_ptr);

2.31.1

This reverts commit 51dd97d1df5fb9ac58b9b358e63e67b530f6ae21.

Because of recent interactions with developers from @umn.edu, all commits from them have been recently re-reviewed to ensure if they were correct or not.

Upon review, this commit was found to be incorrect for the reasons below, so it must be reverted. It will be fixed up "correctly" in a later kernel change.

Lots of things seem to be still allocated here and must be properly cleaned up if an error happens here.

---
 sound/soc/codecs/rt5645.c | 3 ---
 1 file changed, 3 deletions(-)

index 9408ee63cb26..7cb90975009a 100644
@@ -3431,9 +3431,6 @@ static int rt5645_probe(struct snd_soc_component *component)
 		RT5645_HWEQ_NUM, sizeof(struct rt5645_eq_param_s),
 		GFP_KERNEL);
 
-	if (!rt5645->eq_param)
-		return -ENOMEM;
-
 	return 0;
 }
 

2.31.1

From: Phillip Potter <[email protected]>

Check for return value from various snd_soc_dapm_* calls, as many of them can return errors and this should be handled. Also, reintroduce the allocation failure check for rt5645->eq_param as well. Make all areas where return values are checked lead to the end of the function in the case of an error. Finally, introduce a comment explaining how resources here are actually eventually cleaned up by the caller.

---
 sound/soc/codecs/rt5645.c | 48 +++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 9 deletions(-)

index 7cb90975009a..438fa18bcb55 100644
@@ -3388,30 +3388,44 @@ static int rt5645_probe(struct snd_soc_component *component)
 {
 	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
 	struct rt5645_priv *rt5645 = snd_soc_component_get_drvdata(component);
+	int ret = 0;
 
 	rt5645->component = component;
 
 	switch (rt5645->codec_type) {
 	case CODEC_TYPE_RT5645:
-		snd_soc_dapm_new_controls(dapm,
+		ret = snd_soc_dapm_new_controls(dapm,
 			rt5645_specific_dapm_widgets,
 			ARRAY_SIZE(rt5645_specific_dapm_widgets));
-		snd_soc_dapm_add_routes(dapm,
+		if (ret < 0)
+			goto exit;
+
+		ret = snd_soc_dapm_add_routes(dapm,
 			rt5645_specific_dapm_routes,
 			ARRAY_SIZE(rt5645_specific_dapm_routes));
+		if (ret < 0)
+			goto exit;
+
 		if (rt5645->v_id < 3) {
-			snd_soc_dapm_add_routes(dapm,
+			ret = snd_soc_dapm_add_routes(dapm,
 				rt5645_old_dapm_routes,
 				ARRAY_SIZE(rt5645_old_dapm_routes));
+			if (ret < 0)
+				goto exit;
 		}
 		break;
 	case CODEC_TYPE_RT5650:
-		snd_soc_dapm_new_controls(dapm,
+		ret = snd_soc_dapm_new_controls(dapm,
 			rt5650_specific_dapm_widgets,
 			ARRAY_SIZE(rt5650_specific_dapm_widgets));
-		snd_soc_dapm_add_routes(dapm,
+		if (ret < 0)
+			goto exit;
+
+		ret = snd_soc_dapm_add_routes(dapm,
 			rt5650_specific_dapm_routes,
 			ARRAY_SIZE(rt5650_specific_dapm_routes));
+		if (ret < 0)
+			goto exit;
 		break;
 	}
 

@@ -3419,9 +3433,17 @@ static int rt5645_probe(struct snd_soc_component *component)
 
 	/* for JD function */
 	if (rt5645->pdata.jd_mode) {
-		snd_soc_dapm_force_enable_pin(dapm, "JD Power");
-		snd_soc_dapm_force_enable_pin(dapm, "LDO2");
-		snd_soc_dapm_sync(dapm);
+		ret = snd_soc_dapm_force_enable_pin(dapm, "JD Power");
+		if (ret < 0)
+			goto exit;
+
+		ret = snd_soc_dapm_force_enable_pin(dapm, "LDO2");
+		if (ret < 0)
+			goto exit;
+
+		ret = snd_soc_dapm_sync(dapm);
+		if (ret < 0)
+			goto exit;
 	}
 
 	if (rt5645->pdata.long_name)

@@ -3431,7 +3453,15 @@ static int rt5645_probe(struct snd_soc_component *component)
 		RT5645_HWEQ_NUM, sizeof(struct rt5645_eq_param_s),
 		GFP_KERNEL);
 
-	return 0;
+	if (!rt5645->eq_param)
+		ret = -ENOMEM;
+exit:
+	/*
+	 * If there was an error above, everything will be cleaned up by the
+	 * caller if we return an error here.  This will be done with a later
+	 * call to rt5645_remove().
+	 */
+	return ret;
 }
 
 static void rt5645_remove(struct snd_soc_component *component)

2.31.1

Phillip, please follow the standard patch submission process, this is documented in submitting-paches.rst in the kernel tree. In particular please make sure that you copy the relevant maintainers and mailing lists for the subsystem and any driver specific maintainers on any patches that you are submitting to the kernel so that they can be reviewed.