[PATCH 00/69] "Revert and fix properly" patch series based on umn.edu re-review

[PATCH 00/69] "Revert and fix properly" patch series based on umn.edu re-review

[Greg Kroah-Hartman]

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

[PATCH 01/69] Revert "crypto: cavium/nitrox - add an error message to explain the failure of pci_request_mem_regions"

[Greg Kroah-Hartman]

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.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: linux-crypto@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/crypto/cavium/nitrox/nitrox_main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
index facc8e6bc580..d385daf2c71c 100644
--- a/drivers/crypto/cavium/nitrox/nitrox_main.c
+++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
@@ -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

[PATCH 02/69] Revert "ACPI: custom_method: fix memory leaks"

[Greg Kroah-Hartman]

From: Kees Cook <keescook@chromium.org>

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/20110222193250.GA23913@outflux.net/
[2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
[3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/

Cc: Wenwen Wang <wenwen@cs.uga.edu>
Fixes: 03d1571d9513 ("ACPI: custom_method: fix memory leaks")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/acpi/custom_method.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index 443fdf62dd22..72469a49837d 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -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

[PATCH 03/69] Revert "media: rcar_drif: fix a memory disclosure"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Fixes: d39083234c60 ("media: rcar_drif: fix a memory disclosure")
Cc: stable <stable@vger.kernel.org>
Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/media/platform/rcar_drif.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
index 83bd9a412a56..1e3b68a8743a 100644
--- a/drivers/media/platform/rcar_drif.c
+++ b/drivers/media/platform/rcar_drif.c
@@ -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

[PATCH 04/69] Revert "hwmon: (lm80) fix a missing check of bus read in lm80 probe"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Fixes: 9aa3aa15f4c2 ("hwmon: (lm80) fix a missing check of bus read in lm80 probe")
Cc: stable <stable@vger.kernel.org>
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/hwmon/lm80.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/lm80.c b/drivers/hwmon/lm80.c
index ac4adb44b224..97ab491d2922 100644
--- a/drivers/hwmon/lm80.c
+++ b/drivers/hwmon/lm80.c
@@ -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

[PATCH 05/69] Revert "serial: mvebu-uart: Fix to avoid a potential NULL pointer dereference"

[Greg Kroah-Hartman]

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.

Cc: Aditya Pakki <pakki001@umn.edu>
Cc: stable <stable@vger.kernel.org>
Fixes: 32f47179833b ("serial: mvebu-uart: Fix to avoid a potential NULL pointer dereference")
Acked-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/tty/serial/mvebu-uart.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
index e0c00a1b0763..51b0ecabf2ec 100644
--- a/drivers/tty/serial/mvebu-uart.c
+++ b/drivers/tty/serial/mvebu-uart.c
@@ -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

[PATCH 06/69] Revert "media: usb: gspca: add a missed check for goto_low_power"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/media/usb/gspca/cpia1.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/media/usb/gspca/cpia1.c b/drivers/media/usb/gspca/cpia1.c
index a4f7431486f3..d93d384286c1 100644
--- a/drivers/media/usb/gspca/cpia1.c
+++ b/drivers/media/usb/gspca/cpia1.c
@@ -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

[PATCH 07/69] Revert "ALSA: sb: fix a missing check of snd_ctl_add"

[Greg Kroah-Hartman]

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.

Cc: Aditya Pakki <pakki001@umn.edu>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/isa/sb/sb16_main.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/sound/isa/sb/sb16_main.c b/sound/isa/sb/sb16_main.c
index 38dc1fde25f3..aa4870531023 100644
--- a/sound/isa/sb/sb16_main.c
+++ b/sound/isa/sb/sb16_main.c
@@ -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

[PATCH 08/69] Revert "leds: lp5523: fix a missing check of return value of lp55xx_read"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: stable <stable@vger.kernel.org>
Fixes: 248b57015f35 ("leds: lp5523: fix a missing check of return value of lp55xx_read")
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/leds/leds-lp5523.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index fc433e63b1dc..5036d7d5f3d4 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -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

[PATCH 09/69] leds: lp5523: check return value of lp5xx_read and jump to cleanup code

[Greg Kroah-Hartman]

From: Phillip Potter <phil@philpotter.co.uk>

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.

Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/leds/leds-lp5523.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 5036d7d5f3d4..b1590cb4a188 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -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

Re: [PATCH 09/69] leds: lp5523: check return value of lp5xx_read and jump to cleanup code

[Jacek Anaszewski]

On 5/3/21 1:56 PM, Greg Kroah-Hartman wrote:
> From: Phillip Potter <phil@philpotter.co.uk>
> 
> 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.
> 
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>   drivers/leds/leds-lp5523.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index 5036d7d5f3d4..b1590cb4a188 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -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) {
> 

Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

Cc: Pavel Machek <pavel@ucw.cz>, linux-leds@vger.kernel.org

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 09/69] leds: lp5523: check return value of lp5xx_read and jump to cleanup code

[Greg Kroah-Hartman]

On Mon, May 03, 2021 at 09:36:14PM +0200, Jacek Anaszewski wrote:
> On 5/3/21 1:56 PM, Greg Kroah-Hartman wrote:
> > From: Phillip Potter <phil@philpotter.co.uk>
> > 
> > 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.
> > 
> > Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > Cc: stable <stable@vger.kernel.org>
> > Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >   drivers/leds/leds-lp5523.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> > index 5036d7d5f3d4..b1590cb4a188 100644
> > --- a/drivers/leds/leds-lp5523.c
> > +++ b/drivers/leds/leds-lp5523.c
> > @@ -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) {
> > 
> 
> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

Thanks for the review!

[PATCH 10/69] Revert "serial: max310x: pass return value of spi_register_driver"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Acked-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/tty/serial/max310x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 8534d6e45a1d..a3ba0e6520a1 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -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

[PATCH 11/69] serial: max310x: unregister uart driver in case of failure and abort

[Greg Kroah-Hartman]

From: Atul Gopinathan <atulgopinathan@gmail.com>

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.

Cc: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/tty/serial/max310x.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index a3ba0e6520a1..3cbc757d7be7 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -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

[PATCH 12/69] Revert "rtlwifi: fix a potential NULL pointer dereference"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: Bryan Brattlof <hello@bryanbrattlof.com>
Fixes: 765976285a8c ("rtlwifi: fix a potential NULL pointer dereference")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/wireless/realtek/rtlwifi/base.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c
index 2a7ee90a3f54..4136d7c63254 100644
--- a/drivers/net/wireless/realtek/rtlwifi/base.c
+++ b/drivers/net/wireless/realtek/rtlwifi/base.c
@@ -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

[PATCH 13/69] net: rtlwifi: properly check for alloc_workqueue() failure

[Greg Kroah-Hartman]

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

Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: Bryan Brattlof <hello@bryanbrattlof.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/wireless/realtek/rtlwifi/base.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c
index 4136d7c63254..ffd150ec181f 100644
--- a/drivers/net/wireless/realtek/rtlwifi/base.c
+++ b/drivers/net/wireless/realtek/rtlwifi/base.c
@@ -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

[PATCH 14/69] Revert "net: fujitsu: fix a potential NULL pointer dereference"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: David S. Miller <davem@davemloft.net>
Reviewed-by: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/ethernet/fujitsu/fmvj18x_cs.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/fujitsu/fmvj18x_cs.c b/drivers/net/ethernet/fujitsu/fmvj18x_cs.c
index a7b7a4aace79..dc90c61fc827 100644
--- a/drivers/net/ethernet/fujitsu/fmvj18x_cs.c
+++ b/drivers/net/ethernet/fujitsu/fmvj18x_cs.c
@@ -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

[PATCH 15/69] net: fujitsu: fix potential null-ptr-deref

[Greg Kroah-Hartman]

From: Anirudh Rayabharam <mail@anirudhrb.com>

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.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/ethernet/fujitsu/fmvj18x_cs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/fujitsu/fmvj18x_cs.c b/drivers/net/ethernet/fujitsu/fmvj18x_cs.c
index dc90c61fc827..b0c0504950d8 100644
--- a/drivers/net/ethernet/fujitsu/fmvj18x_cs.c
+++ b/drivers/net/ethernet/fujitsu/fmvj18x_cs.c
@@ -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

[PATCH 16/69] Revert "net/smc: fix a NULL pointer dereference"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Ursula Braun <ubraun@linux.ibm.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 net/smc/smc_ism.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/net/smc/smc_ism.c b/net/smc/smc_ism.c
index 9c6e95882553..6558cf7643a7 100644
--- a/net/smc/smc_ism.c
+++ b/net/smc/smc_ism.c
@@ -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

[PATCH 17/69] net/smc: properly handle workqueue allocation failure

[Greg Kroah-Hartman]

From: Anirudh Rayabharam <mail@anirudhrb.com>

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().

Cc: Ursula Braun <ubraun@linux.ibm.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 net/smc/smc_ism.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/smc/smc_ism.c b/net/smc/smc_ism.c
index 6558cf7643a7..94b31f2551bc 100644
--- a/net/smc/smc_ism.c
+++ b/net/smc/smc_ism.c
@@ -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

[PATCH 18/69] Revert "net: caif: replace BUG_ON with recovery code"

[Greg Kroah-Hartman]

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.

Cc: Aditya Pakki <pakki001@umn.edu>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/caif/caif_serial.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/caif/caif_serial.c b/drivers/net/caif/caif_serial.c
index da6fffb4d5a8..a7f51eb58915 100644
--- a/drivers/net/caif/caif_serial.c
+++ b/drivers/net/caif/caif_serial.c
@@ -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

[PATCH 19/69] net: caif: remove BUG_ON(dev == NULL) in caif_xmit

[Greg Kroah-Hartman]

From: Du Cheng <ducheng2@gmail.com>

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.

Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Du Cheng <ducheng2@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/caif/caif_serial.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/caif/caif_serial.c b/drivers/net/caif/caif_serial.c
index a7f51eb58915..d17482395a4d 100644
--- a/drivers/net/caif/caif_serial.c
+++ b/drivers/net/caif/caif_serial.c
@@ -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

[PATCH 20/69] Revert "net: stmicro: fix a missing check of clk_prepare"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: David S. Miller <davem@davemloft.net>
Fixes: f86a3b83833e ("net: stmicro: fix a missing check of clk_prepare")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
index 527077c98ebc..fc68e90acbea 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
@@ -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

[PATCH 21/69] net: stmicro: handle clk_prepare() failure during init

[Greg Kroah-Hartman]

From: Anirudh Rayabharam <mail@anirudhrb.com>

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().

Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
index fc68e90acbea..fc3b0acc8f99 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
@@ -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

[PATCH 22/69] Revert "niu: fix missing checks of niu_pci_eeprom_read"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Shannon Nelson <shannon.lee.nelson@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Fixes: 26fd962bde0b ("niu: fix missing checks of niu_pci_eeprom_read")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/ethernet/sun/niu.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index 707ccdd03b19..d70cdea756d1 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -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

[PATCH 23/69] ethernet: sun: niu: fix missing checks of niu_pci_eeprom_read()

[Greg Kroah-Hartman]

From: Du Cheng <ducheng2@gmail.com>

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.

Signed-off-by: Du Cheng <ducheng2@gmail.com>
Cc: Shannon Nelson <shannon.lee.nelson@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/ethernet/sun/niu.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index d70cdea756d1..74e748662ec0 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -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

[PATCH 24/69] Revert "qlcnic: Avoid potential NULL pointer dereference"

[Greg Kroah-Hartman]

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.

Cc: Aditya Pakki <pakki001@umn.edu>
Cc: David S. Miller <davem@davemloft.net>
Fixes: 5bf7295fe34a ("qlcnic: Avoid potential NULL pointer dereference")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
index d8a3ecaed3fc..985cf8cb2ec0 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
@@ -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

[PATCH 25/69] qlcnic: Add null check after calling netdev_alloc_skb

[Greg Kroah-Hartman]

From: Tom Seewald <tseewald@gmail.com>

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.

Cc: David S. Miller <davem@davemloft.net>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Tom Seewald <tseewald@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
index 985cf8cb2ec0..d8f0863b3934 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
@@ -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

[PATCH 26/69] Revert "gdrom: fix a memory leak bug"

[Greg Kroah-Hartman]

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.

Cc: Wenwen Wang <wang6495@umn.edu>
Cc: Peter Rosin <peda@axentia.se>
Cc: Jens Axboe <axboe@kernel.dk>
Fixes: 093c48213ee3 ("gdrom: fix a memory leak bug")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/cdrom/gdrom.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
index 742b4a0932e3..7f681320c7d3 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -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

[PATCH 27/69] cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom

[Greg Kroah-Hartman]

From: Atul Gopinathan <atulgopinathan@gmail.com>

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".

Suggested-by: Jens Axboe <axboe@kernel.dk>
Cc: Peter Rosin <peda@axentia.se>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/cdrom/gdrom.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
index 7f681320c7d3..6c4f6139f853 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -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

Re: [PATCH 27/69] cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom

[Peter Rosin]

Hi!

On 2021-05-03 13:56, Greg Kroah-Hartman wrote:
> From: Atul Gopinathan <atulgopinathan@gmail.com>
> 
> 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".
> 
> Suggested-by: Jens Axboe <axboe@kernel.dk>
> Cc: Peter Rosin <peda@axentia.se>
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/cdrom/gdrom.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
> index 7f681320c7d3..6c4f6139f853 100644
> --- a/drivers/cdrom/gdrom.c
> +++ b/drivers/cdrom/gdrom.c
> @@ -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);
> 

I worry about the gd.toc = NULL; statement in init_gdrom(). It sets off
all kinds of warnings with me. It looks completely bogus, but the fact
that it's there at all makes me go hmmmm.

probe_gdrom_setupcd() will arrange for gdrom_ops to be used, including
.get_last_session pointing to gdrom_get_last_session() 

gdrom_get_last_session() will use gd.toc, if it is non-NULL.

The above will all be registered externally to the driver with the call
to register_cdrom() in probe_gdrom(), before a possible stale gd.toc is
overwritten with a new one at the end of probe_gdrom().

Side note, .get_last_session is an interesting name in this context, but
I have no idea if it might be called in the "bad" window (but relying on
that to not be the case would be ... subtle).

So, by simply freeing gd.toc in remove_gdrom() without also setting
it to NULL, it looks like a potential use after free of gd.toc is
introduced, replacing a potential leak. Not good.

The same is not true for gd.cd_info as far as I can tell, but it's a bit
subtle. gdrom_probe() calls gdrom_execute_diagnostics() before the stale
gd.cd_info is overwritten, and gdrom_execute_diagnostic() passes the
stale pointer to gdrom_hardreset(), which luckily doesn't use it. But
this is - as hinted - a bit too subtle for me. I would prefer to have
remove_gdrom() also clear out the gd.cd_info pointer.

In addition to adding these clears of gd.toc and gd.cd_info to
remove_gdrom(), they also need to be cleared in case probe fails.

Or instead, maybe add a big fat
	memset(&gd, 0, sizeof(gd));
at the top of probe?
Or maybe the struct gdrom_unit should simply be kzalloc:ed? But that
triggers some . to -> churn...

Anyway, the patch as proposed gets a NACK from me.

Cheers,
Peter

Re: [PATCH 27/69] cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom

[Greg Kroah-Hartman]

On Mon, May 03, 2021 at 04:13:18PM +0200, Peter Rosin wrote:
> Hi!
> 
> On 2021-05-03 13:56, Greg Kroah-Hartman wrote:
> > From: Atul Gopinathan <atulgopinathan@gmail.com>
> > 
> > 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".
> > 
> > Suggested-by: Jens Axboe <axboe@kernel.dk>
> > Cc: Peter Rosin <peda@axentia.se>
> > Cc: stable <stable@vger.kernel.org>
> > Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/cdrom/gdrom.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
> > index 7f681320c7d3..6c4f6139f853 100644
> > --- a/drivers/cdrom/gdrom.c
> > +++ b/drivers/cdrom/gdrom.c
> > @@ -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);
> > 
> 
> I worry about the gd.toc = NULL; statement in init_gdrom(). It sets off
> all kinds of warnings with me. It looks completely bogus, but the fact
> that it's there at all makes me go hmmmm.

Yeah, that's bogus.

> probe_gdrom_setupcd() will arrange for gdrom_ops to be used, including
> .get_last_session pointing to gdrom_get_last_session() 
> 
> gdrom_get_last_session() will use gd.toc, if it is non-NULL.
> 
> The above will all be registered externally to the driver with the call
> to register_cdrom() in probe_gdrom(), before a possible stale gd.toc is
> overwritten with a new one at the end of probe_gdrom().

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

> Side note, .get_last_session is an interesting name in this context, but
> I have no idea if it might be called in the "bad" window (but relying on
> that to not be the case would be ... subtle).
> 
> So, by simply freeing gd.toc in remove_gdrom() without also setting
> it to NULL, it looks like a potential use after free of gd.toc is
> introduced, replacing a potential leak. Not good.

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.

> The same is not true for gd.cd_info as far as I can tell, but it's a bit
> subtle. gdrom_probe() calls gdrom_execute_diagnostics() before the stale
> gd.cd_info is overwritten, and gdrom_execute_diagnostic() passes the
> stale pointer to gdrom_hardreset(), which luckily doesn't use it. But
> this is - as hinted - a bit too subtle for me. I would prefer to have
> remove_gdrom() also clear out the gd.cd_info pointer.

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

> In addition to adding these clears of gd.toc and gd.cd_info to
> remove_gdrom(), they also need to be cleared in case probe fails.
> 
> Or instead, maybe add a big fat
> 	memset(&gd, 0, sizeof(gd));
> at the top of probe?

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.

> Or maybe the struct gdrom_unit should simply be kzalloc:ed? But that
> triggers some . to -> churn...

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.

> Anyway, the patch as proposed gets a NACK from me.

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

Re: [PATCH 27/69] cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom

[Peter Rosin]

Hi!

On 2021-05-06 12:24, Greg Kroah-Hartman wrote:
> On Mon, May 03, 2021 at 04:13:18PM +0200, Peter Rosin wrote:
>> Hi!
>>
>> On 2021-05-03 13:56, Greg Kroah-Hartman wrote:
>>> From: Atul Gopinathan <atulgopinathan@gmail.com>
>>>
>>> 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".
>>>
>>> Suggested-by: Jens Axboe <axboe@kernel.dk>
>>> Cc: Peter Rosin <peda@axentia.se>
>>> Cc: stable <stable@vger.kernel.org>
>>> Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com>
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> ---
>>>  drivers/cdrom/gdrom.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
>>> index 7f681320c7d3..6c4f6139f853 100644
>>> --- a/drivers/cdrom/gdrom.c
>>> +++ b/drivers/cdrom/gdrom.c
>>> @@ -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);
>>>
>>
>> I worry about the gd.toc = NULL; statement in init_gdrom(). It sets off
>> all kinds of warnings with me. It looks completely bogus, but the fact
>> that it's there at all makes me go hmmmm.
> 
> Yeah, that's bogus.
> 
>> probe_gdrom_setupcd() will arrange for gdrom_ops to be used, including
>> .get_last_session pointing to gdrom_get_last_session() 
>>
>> gdrom_get_last_session() will use gd.toc, if it is non-NULL.
>>
>> The above will all be registered externally to the driver with the call
>> to register_cdrom() in probe_gdrom(), before a possible stale gd.toc is
>> overwritten with a new one at the end of probe_gdrom().
> 
> But can that really happen given that it hasn't ever happened before in
> a real system?  :)
> 
>> Side note, .get_last_session is an interesting name in this context, but
>> I have no idea if it might be called in the "bad" window (but relying on
>> that to not be the case would be ... subtle).
>>
>> So, by simply freeing gd.toc in remove_gdrom() without also setting
>> it to NULL, it looks like a potential use after free of gd.toc is
>> introduced, replacing a potential leak. Not good.
> 
> 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.
> 
>> The same is not true for gd.cd_info as far as I can tell, but it's a bit
>> subtle. gdrom_probe() calls gdrom_execute_diagnostics() before the stale
>> gd.cd_info is overwritten, and gdrom_execute_diagnostic() passes the
>> stale pointer to gdrom_hardreset(), which luckily doesn't use it. But
>> this is - as hinted - a bit too subtle for me. I would prefer to have
>> remove_gdrom() also clear out the gd.cd_info pointer.
> 
> Ok, but again, how can that be used after remove_gdrom() is called?
> 
>> In addition to adding these clears of gd.toc and gd.cd_info to
>> remove_gdrom(), they also need to be cleared in case probe fails.
>>
>> Or instead, maybe add a big fat
>> 	memset(&gd, 0, sizeof(gd));
>> at the top of probe?
> 
> 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.
> 
>> Or maybe the struct gdrom_unit should simply be kzalloc:ed? But that
>> triggers some . to -> churn...
> 
> 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.
> 
>> Anyway, the patch as proposed gets a NACK from me.
> 
> 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.

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

Re: [PATCH 27/69] cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom

[Greg Kroah-Hartman]

On Thu, May 06, 2021 at 03:08:08PM +0200, Peter Rosin wrote:
> Hi!
> 
> On 2021-05-06 12:24, Greg Kroah-Hartman wrote:
> > On Mon, May 03, 2021 at 04:13:18PM +0200, Peter Rosin wrote:
> >> Hi!
> >>
> >> On 2021-05-03 13:56, Greg Kroah-Hartman wrote:
> >>> From: Atul Gopinathan <atulgopinathan@gmail.com>
> >>>
> >>> 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".
> >>>
> >>> Suggested-by: Jens Axboe <axboe@kernel.dk>
> >>> Cc: Peter Rosin <peda@axentia.se>
> >>> Cc: stable <stable@vger.kernel.org>
> >>> Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com>
> >>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>> ---
> >>>  drivers/cdrom/gdrom.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
> >>> index 7f681320c7d3..6c4f6139f853 100644
> >>> --- a/drivers/cdrom/gdrom.c
> >>> +++ b/drivers/cdrom/gdrom.c
> >>> @@ -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);
> >>>
> >>
> >> I worry about the gd.toc = NULL; statement in init_gdrom(). It sets off
> >> all kinds of warnings with me. It looks completely bogus, but the fact
> >> that it's there at all makes me go hmmmm.
> > 
> > Yeah, that's bogus.
> > 
> >> probe_gdrom_setupcd() will arrange for gdrom_ops to be used, including
> >> .get_last_session pointing to gdrom_get_last_session() 
> >>
> >> gdrom_get_last_session() will use gd.toc, if it is non-NULL.
> >>
> >> The above will all be registered externally to the driver with the call
> >> to register_cdrom() in probe_gdrom(), before a possible stale gd.toc is
> >> overwritten with a new one at the end of probe_gdrom().
> > 
> > But can that really happen given that it hasn't ever happened before in
> > a real system?  :)
> > 
> >> Side note, .get_last_session is an interesting name in this context, but
> >> I have no idea if it might be called in the "bad" window (but relying on
> >> that to not be the case would be ... subtle).
> >>
> >> So, by simply freeing gd.toc in remove_gdrom() without also setting
> >> it to NULL, it looks like a potential use after free of gd.toc is
> >> introduced, replacing a potential leak. Not good.
> > 
> > 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.
> > 
> >> The same is not true for gd.cd_info as far as I can tell, but it's a bit
> >> subtle. gdrom_probe() calls gdrom_execute_diagnostics() before the stale
> >> gd.cd_info is overwritten, and gdrom_execute_diagnostic() passes the
> >> stale pointer to gdrom_hardreset(), which luckily doesn't use it. But
> >> this is - as hinted - a bit too subtle for me. I would prefer to have
> >> remove_gdrom() also clear out the gd.cd_info pointer.
> > 
> > Ok, but again, how can that be used after remove_gdrom() is called?
> > 
> >> In addition to adding these clears of gd.toc and gd.cd_info to
> >> remove_gdrom(), they also need to be cleared in case probe fails.
> >>
> >> Or instead, maybe add a big fat
> >> 	memset(&gd, 0, sizeof(gd));
> >> at the top of probe?
> > 
> > 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.
> > 
> >> Or maybe the struct gdrom_unit should simply be kzalloc:ed? But that
> >> triggers some . to -> churn...
> > 
> > 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.
> > 
> >> Anyway, the patch as proposed gets a NACK from me.
> > 
> > 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.
> 
> 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.

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.

> 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.

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

[PATCH] cdrom: gdrom: initialize global variable at init time

[Greg Kroah-Hartman]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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.

Reported-by: Peter Rosin <peda@axentia.se>
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---

Note, this goes on top of my previous "gdrom" patch submitted here:
	https://lore.kernel.org/lkml/20210503115736.2104747-28-gregkh@linuxfoundation.org/

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

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

diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
index 6c4f6139f853..c6d8c0f59722 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -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

Re: [PATCH] cdrom: gdrom: initialize global variable at init time

[Peter Rosin]

Hi!

On 2021-05-06 16:00, Greg Kroah-Hartman wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> 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.
> 
> Reported-by: Peter Rosin <peda@axentia.se>
> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Looks good to me.

Reviewed-by: Peter Rosin <peda@axentia.se>

Thanks,
Peter

Re: [PATCH 27/69] cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom

[Atul Gopinathan]

On Thu, May 06, 2021 at 03:08:08PM +0200, Peter Rosin wrote:
> Hi!
> 
> On 2021-05-06 12:24, Greg Kroah-Hartman wrote:
> > On Mon, May 03, 2021 at 04:13:18PM +0200, Peter Rosin wrote:
> >> Hi!
> >>
> >> On 2021-05-03 13:56, Greg Kroah-Hartman wrote:
> >>> From: Atul Gopinathan <atulgopinathan@gmail.com>
> >>>
> >>> 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".
> >>>
> >>> Suggested-by: Jens Axboe <axboe@kernel.dk>
> >>> Cc: Peter Rosin <peda@axentia.se>
> >>> Cc: stable <stable@vger.kernel.org>
> >>> Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com>
> >>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>> ---
> >>>  drivers/cdrom/gdrom.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
> >>> index 7f681320c7d3..6c4f6139f853 100644
> >>> --- a/drivers/cdrom/gdrom.c
> >>> +++ b/drivers/cdrom/gdrom.c
> >>> @@ -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);
> >>>
> >>
> >> I worry about the gd.toc = NULL; statement in init_gdrom(). It sets off
> >> all kinds of warnings with me. It looks completely bogus, but the fact
> >> that it's there at all makes me go hmmmm.
> > 
> > Yeah, that's bogus.
> > 
> >> probe_gdrom_setupcd() will arrange for gdrom_ops to be used, including
> >> .get_last_session pointing to gdrom_get_last_session() 
> >>
> >> gdrom_get_last_session() will use gd.toc, if it is non-NULL.
> >>
> >> The above will all be registered externally to the driver with the call
> >> to register_cdrom() in probe_gdrom(), before a possible stale gd.toc is
> >> overwritten with a new one at the end of probe_gdrom().
> > 
> > But can that really happen given that it hasn't ever happened before in
> > a real system?  :)
> > 
> >> Side note, .get_last_session is an interesting name in this context, but
> >> I have no idea if it might be called in the "bad" window (but relying on
> >> that to not be the case would be ... subtle).
> >>
> >> So, by simply freeing gd.toc in remove_gdrom() without also setting
> >> it to NULL, it looks like a potential use after free of gd.toc is
> >> introduced, replacing a potential leak. Not good.
> > 
> > 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.
> > 
> >> The same is not true for gd.cd_info as far as I can tell, but it's a bit
> >> subtle. gdrom_probe() calls gdrom_execute_diagnostics() before the stale
> >> gd.cd_info is overwritten, and gdrom_execute_diagnostic() passes the
> >> stale pointer to gdrom_hardreset(), which luckily doesn't use it. But
> >> this is - as hinted - a bit too subtle for me. I would prefer to have
> >> remove_gdrom() also clear out the gd.cd_info pointer.
> > 
> > Ok, but again, how can that be used after remove_gdrom() is called?
> > 
> >> In addition to adding these clears of gd.toc and gd.cd_info to
> >> remove_gdrom(), they also need to be cleared in case probe fails.
> >>
> >> Or instead, maybe add a big fat
> >> 	memset(&gd, 0, sizeof(gd));
> >> at the top of probe?
> > 
> > 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.
> > 
> >> Or maybe the struct gdrom_unit should simply be kzalloc:ed? But that
> >> triggers some . to -> churn...
> > 
> > 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.
> > 
> >> Anyway, the patch as proposed gets a NACK from me.
> > 
> > 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.
> 
> 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 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

Re: [PATCH 27/69] cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom

[Peter Rosin]

Hi!

On 2021-05-06 16:32, Atul Gopinathan wrote:
> 
> 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.

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

Re: [PATCH 27/69] cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom

[Atul Gopinathan]

On Thu, May 06, 2021 at 05:43:14PM +0200, Peter Rosin wrote:
> Hi!
> 
> On 2021-05-06 16:32, Atul Gopinathan wrote:
> > 
> > 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.
> 
> 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.

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

Regards,
Atul

[PATCH 28/69] Revert "char: hpet: fix a missing check of ioremap"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/char/hpet.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index ed3b7dab678d..6f13def6c172 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -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

[PATCH 29/69] char: hpet: add checks after calling ioremap

[Greg Kroah-Hartman]

From: Tom Seewald <tseewald@gmail.com>

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.

Signed-off-by: Tom Seewald <tseewald@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/char/hpet.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index 6f13def6c172..8b55085650ad 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -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

[PATCH 30/69] Revert "scsi: ufs: fix a missing check of devm_reset_control_get"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Fixes: 63a06181d7ce ("scsi: ufs: fix a missing check of devm_reset_control_get")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/scsi/ufs/ufs-hisi.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c
index 0aa58131e791..7d1e07a9d9dd 100644
--- a/drivers/scsi/ufs/ufs-hisi.c
+++ b/drivers/scsi/ufs/ufs-hisi.c
@@ -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

[PATCH 31/69] scsi: ufs: handle cleanup correctly on devm_reset_control_get error

[Greg Kroah-Hartman]

From: Phillip Potter <phil@philpotter.co.uk>

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.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/scsi/ufs/ufs-hisi.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c
index 7d1e07a9d9dd..d0626773eb38 100644
--- a/drivers/scsi/ufs/ufs-hisi.c
+++ b/drivers/scsi/ufs/ufs-hisi.c
@@ -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

[PATCH 32/69] Revert "ALSA: gus: add a check of the status of snd_ctl_add"

[Greg Kroah-Hartman]

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".

Cc: Kangjie Lu <kjlu@umn.edu>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/isa/gus/gus_main.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/sound/isa/gus/gus_main.c b/sound/isa/gus/gus_main.c
index afc088f0377c..b7518122a10d 100644
--- a/sound/isa/gus/gus_main.c
+++ b/sound/isa/gus/gus_main.c
@@ -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

[PATCH 33/69] ALSA: gus: properly handle snd_ctl_add() error

[Greg Kroah-Hartman]

From: Kurt Manucredo <fuzzybritches0@gmail.com>

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.

Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Kurt Manucredo <fuzzybritches0@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/isa/gus/gus_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/isa/gus/gus_main.c b/sound/isa/gus/gus_main.c
index b7518122a10d..4c2703ea55fb 100644
--- a/sound/isa/gus/gus_main.c
+++ b/sound/isa/gus/gus_main.c
@@ -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

Re: [PATCH 33/69] ALSA: gus: properly handle snd_ctl_add() error

[Takashi Iwai]

On Mon, 03 May 2021 13:57:00 +0200,
Greg Kroah-Hartman wrote:
> 
> From: Kurt Manucredo <fuzzybritches0@gmail.com>
> 
> 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.
> 
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Kurt Manucredo <fuzzybritches0@gmail.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

> ---
>  sound/isa/gus/gus_main.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/isa/gus/gus_main.c b/sound/isa/gus/gus_main.c
> index b7518122a10d..4c2703ea55fb 100644
> --- a/sound/isa/gus/gus_main.c
> +++ b/sound/isa/gus/gus_main.c
> @@ -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
> 

Re: [PATCH 33/69] ALSA: gus: properly handle snd_ctl_add() error

[Greg Kroah-Hartman]

On Mon, May 03, 2021 at 02:28:25PM +0200, Takashi Iwai wrote:
> On Mon, 03 May 2021 13:57:00 +0200,
> Greg Kroah-Hartman wrote:
> > 
> > From: Kurt Manucredo <fuzzybritches0@gmail.com>
> > 
> > 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.
> > 
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Kurt Manucredo <fuzzybritches0@gmail.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> 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 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

[PATCH 34/69] Revert "ALSA: sb8: add a check for request_region"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Acked-by: Takashi Iwai <tiwai@suse.de>
Fixes: dcd0feac9bab ("ALSA: sb8: add a check for request_region")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/isa/sb/sb8.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/sound/isa/sb/sb8.c b/sound/isa/sb/sb8.c
index 6c9d534ce8b6..95290ffe5c6e 100644
--- a/sound/isa/sb/sb8.c
+++ b/sound/isa/sb/sb8.c
@@ -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

[PATCH 35/69] ALSA: sb8: Add a comment note regarding an unused pointer

[Greg Kroah-Hartman]

From: Atul Gopinathan <atulgopinathan@gmail.com>

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.

Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/isa/sb/sb8.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/isa/sb/sb8.c b/sound/isa/sb/sb8.c
index 95290ffe5c6e..ed3a87ebe3f4 100644
--- a/sound/isa/sb/sb8.c
+++ b/sound/isa/sb/sb8.c
@@ -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

[PATCH 36/69] Revert "ALSA: usx2y: Fix potential NULL pointer dereference"

[Greg Kroah-Hartman]

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.

Cc: Aditya Pakki <pakki001@umn.edu>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/usb/usx2y/usb_stream.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/sound/usb/usx2y/usb_stream.c b/sound/usb/usx2y/usb_stream.c
index 091c071b270a..6bba17bf689a 100644
--- a/sound/usb/usx2y/usb_stream.c
+++ b/sound/usb/usx2y/usb_stream.c
@@ -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

[PATCH 37/69] ALSA: usx2y: check for failure of usb_alloc_urb()

[Greg Kroah-Hartman]

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.

Cc: Takashi Iwai <tiwai@suse.de>
Cc: Jaroslav Kysela <perex@perex.cz>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/usb/usx2y/usb_stream.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/sound/usb/usx2y/usb_stream.c b/sound/usb/usx2y/usb_stream.c
index 6bba17bf689a..1091ea96a29a 100644
--- a/sound/usb/usx2y/usb_stream.c
+++ b/sound/usb/usx2y/usb_stream.c
@@ -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

Re: [PATCH 37/69] ALSA: usx2y: check for failure of usb_alloc_urb()

[Jaroslav Kysela]

Dne 03. 05. 21 v 13:57 Greg Kroah-Hartman napsal(a):
> 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.

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

> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  sound/usb/usx2y/usb_stream.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/usb/usx2y/usb_stream.c b/sound/usb/usx2y/usb_stream.c
> index 6bba17bf689a..1091ea96a29a 100644
> --- a/sound/usb/usx2y/usb_stream.c
> +++ b/sound/usb/usx2y/usb_stream.c
> @@ -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;
>  

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH 37/69] ALSA: usx2y: check for failure of usb_alloc_urb()

[Takashi Iwai]

On Mon, 03 May 2021 22:33:52 +0200,
Jaroslav Kysela wrote:
> 
> Dne 03. 05. 21 v 13:57 Greg Kroah-Hartman napsal(a):
> > 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.
> 
> 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.

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

> 
> 						Jaroslav
> 
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Cc: Jaroslav Kysela <perex@perex.cz>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  sound/usb/usx2y/usb_stream.c | 21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/usb/usx2y/usb_stream.c b/sound/usb/usx2y/usb_stream.c
> > index 6bba17bf689a..1091ea96a29a 100644
> > --- a/sound/usb/usx2y/usb_stream.c
> > +++ b/sound/usb/usx2y/usb_stream.c
> > @@ -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;
> >  
> 
> -- 
> Jaroslav Kysela <perex@perex.cz>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
> 

Re: [PATCH 37/69] ALSA: usx2y: check for failure of usb_alloc_urb()

[Greg Kroah-Hartman]

On Tue, May 04, 2021 at 10:27:34AM +0200, Takashi Iwai wrote:
> On Mon, 03 May 2021 22:33:52 +0200,
> Jaroslav Kysela wrote:
> > 
> > Dne 03. 05. 21 v 13:57 Greg Kroah-Hartman napsal(a):
> > > 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.
> > 
> > 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.
> 
> Right, the suggested patch will cause the double-free, and the
> reverted code is fine in this regard.

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

> 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?

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

greg k-h

[PATCH 38/69] Revert "video: hgafb: fix potential NULL pointer dereference"

[Greg Kroah-Hartman]

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...

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Aditya Pakki <pakki001@umn.edu>
Cc: Ferenc Bakonyi <fero@drama.obuda.kando.hu>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Fixes: ec7f6aad57ad ("video: hgafb: fix potential NULL pointer dereference")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/hgafb.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/video/fbdev/hgafb.c b/drivers/video/fbdev/hgafb.c
index 8bbac7182ad3..fca29f219f8b 100644
--- a/drivers/video/fbdev/hgafb.c
+++ b/drivers/video/fbdev/hgafb.c
@@ -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

[PATCH 39/69] video: hgafb: fix potential NULL pointer dereference

[Greg Kroah-Hartman]

From: Igor Matheus Andrade Torrente <igormtorrente@gmail.com>

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.

Cc: Ferenc Bakonyi <fero@drama.obuda.kando.hu>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/hgafb.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/video/fbdev/hgafb.c b/drivers/video/fbdev/hgafb.c
index fca29f219f8b..cc8e62ae93f6 100644
--- a/drivers/video/fbdev/hgafb.c
+++ b/drivers/video/fbdev/hgafb.c
@@ -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

[PATCH 40/69] Revert "isdn: mISDNinfineon: fix potential NULL pointer dereference"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/isdn/hardware/mISDN/mISDNinfineon.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/isdn/hardware/mISDN/mISDNinfineon.c b/drivers/isdn/hardware/mISDN/mISDNinfineon.c
index a16c7a2a7f3d..fa9c491f9c38 100644
--- a/drivers/isdn/hardware/mISDN/mISDNinfineon.c
+++ b/drivers/isdn/hardware/mISDN/mISDNinfineon.c
@@ -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

[PATCH 41/69] isdn: mISDNinfineon: check/cleanup ioremap failure correctly in setup_io

[Greg Kroah-Hartman]

From: Phillip Potter <phil@philpotter.co.uk>

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.

Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/isdn/hardware/mISDN/mISDNinfineon.c | 24 ++++++++++++++-------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/isdn/hardware/mISDN/mISDNinfineon.c b/drivers/isdn/hardware/mISDN/mISDNinfineon.c
index fa9c491f9c38..88d592bafdb0 100644
--- a/drivers/isdn/hardware/mISDN/mISDNinfineon.c
+++ b/drivers/isdn/hardware/mISDN/mISDNinfineon.c
@@ -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

[PATCH 42/69] Revert "ath6kl: return error code in ath6kl_wmi_set_roam_lrssi_cmd()"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Kalle Valo <kvalo@codeaurora.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/wireless/ath/ath6kl/wmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index b137e7f34397..aca9732ec1ee 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -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

[PATCH 43/69] ath6kl: return error code in ath6kl_wmi_set_roam_lrssi_cmd()

[Greg Kroah-Hartman]

From: Anirudh Rayabharam <mail@anirudhrb.com>

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

Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/wireless/ath/ath6kl/debug.c | 5 ++++-
 drivers/net/wireless/ath/ath6kl/wmi.c   | 4 +---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
index 7506cea46f58..433a047f3747 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.c
+++ b/drivers/net/wireless/ath/ath6kl/debug.c
@@ -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;
 }
 
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index aca9732ec1ee..b137e7f34397 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -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

[PATCH 44/69] Revert "rapidio: fix a NULL pointer dereference when create_workqueue() fails"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Alexandre Bounine <alex.bou9@gmail.com>
Cc: Matt Porter <mporter@kernel.crashing.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Fixes: 23015b22e47c ("rapidio: fix a NULL pointer dereference when create_workqueue() fails")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/rapidio/rio_cm.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/rapidio/rio_cm.c b/drivers/rapidio/rio_cm.c
index 50ec53d67a4c..e6c16f04f2b4 100644
--- a/drivers/rapidio/rio_cm.c
+++ b/drivers/rapidio/rio_cm.c
@@ -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

[PATCH 45/69] rapidio: handle create_workqueue() failure

[Greg Kroah-Hartman]

From: Anirudh Rayabharam <mail@anirudhrb.com>

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().

Cc: Alexandre Bounine <alex.bou9@gmail.com>
Cc: Matt Porter <mporter@kernel.crashing.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/rapidio/rio_cm.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/rapidio/rio_cm.c b/drivers/rapidio/rio_cm.c
index e6c16f04f2b4..db4c265287ae 100644
--- a/drivers/rapidio/rio_cm.c
+++ b/drivers/rapidio/rio_cm.c
@@ -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

[PATCH 46/69] Revert "isdn: mISDN: Fix potential NULL pointer dereference of kzalloc"

[Greg Kroah-Hartman]

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.

Cc: Aditya Pakki <pakki001@umn.edu>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/isdn/hardware/mISDN/hfcsusb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/isdn/hardware/mISDN/hfcsusb.c b/drivers/isdn/hardware/mISDN/hfcsusb.c
index 70061991915a..4bb470d3963d 100644
--- a/drivers/isdn/hardware/mISDN/hfcsusb.c
+++ b/drivers/isdn/hardware/mISDN/hfcsusb.c
@@ -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

[PATCH 47/69] isdn: mISDN: correctly handle ph_info allocation failure in hfcsusb_ph_info

[Greg Kroah-Hartman]

From: Phillip Potter <phil@philpotter.co.uk>

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.

Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/isdn/hardware/mISDN/hfcsusb.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/isdn/hardware/mISDN/hfcsusb.c b/drivers/isdn/hardware/mISDN/hfcsusb.c
index 4bb470d3963d..cd5642cef01f 100644
--- a/drivers/isdn/hardware/mISDN/hfcsusb.c
+++ b/drivers/isdn/hardware/mISDN/hfcsusb.c
@@ -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

[PATCH 48/69] Revert "ecryptfs: replace BUG_ON with error handling code"

[Greg Kroah-Hartman]

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.

Cc: Aditya Pakki <pakki001@umn.edu>
Fixes: 2c2a7552dd64 ("ecryptfs: replace BUG_ON with error handling code")
Cc: stable <stable@vger.kernel.org>
Acked-by: Tyler Hicks <code@tyhicks.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/ecryptfs/crypto.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index 943e523f4c9d..3d8623139538 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -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

[PATCH 49/69] fs: ecryptfs: remove BUG_ON from crypt_scatterlist

[Greg Kroah-Hartman]

From: Phillip Potter <phil@philpotter.co.uk>

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).

Cc: Tyler Hicks <code@tyhicks.com>
Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/ecryptfs/crypto.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index 3d8623139538..271064511c1b 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -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

[PATCH 50/69] Revert "dmaengine: qcom_hidma: Check for driver register failure"

[Greg Kroah-Hartman]

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().

Cc: Aditya Pakki <pakki001@umn.edu>
Cc: Sinan Kaya <okaya@kernel.org>
Cc: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/dma/qcom/hidma_mgmt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/qcom/hidma_mgmt.c b/drivers/dma/qcom/hidma_mgmt.c
index 806ca02c52d7..fe87b01f7a4e 100644
--- a/drivers/dma/qcom/hidma_mgmt.c
+++ b/drivers/dma/qcom/hidma_mgmt.c
@@ -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

Re: [PATCH 50/69] Revert "dmaengine: qcom_hidma: Check for driver register failure"

[Sinan Kaya]

On 5/3/2021 7:57 AM, Greg Kroah-Hartman wrote:
> 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().
> 
> Cc: Aditya Pakki <pakki001@umn.edu>
> Cc: Sinan Kaya <okaya@kernel.org>
> Cc: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/dma/qcom/hidma_mgmt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/qcom/hidma_mgmt.c b/drivers/dma/qcom/hidma_mgmt.c
> index 806ca02c52d7..fe87b01f7a4e 100644
> --- a/drivers/dma/qcom/hidma_mgmt.c
> +++ b/drivers/dma/qcom/hidma_mgmt.c
> @@ -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");
> 

Acked-By: Sinan Kaya <okaya@kernel.org>

Re: [PATCH 50/69] Revert "dmaengine: qcom_hidma: Check for driver register failure"

[Vinod Koul]

On 03-05-21, 13:57, Greg Kroah-Hartman wrote:
> 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().

Acked-By: Vinod Koul <vkoul@kernel.org>

-- 
~Vinod

[PATCH 51/69] dmaengine: qcom_hidma: comment platform_driver_register call

[Greg Kroah-Hartman]

From: Phillip Potter <phil@philpotter.co.uk>

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.

Cc: Sinan Kaya <okaya@kernel.org>
Cc: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/dma/qcom/hidma_mgmt.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/dma/qcom/hidma_mgmt.c b/drivers/dma/qcom/hidma_mgmt.c
index fe87b01f7a4e..62026607f3f8 100644
--- a/drivers/dma/qcom/hidma_mgmt.c
+++ b/drivers/dma/qcom/hidma_mgmt.c
@@ -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

Re: [PATCH 51/69] dmaengine: qcom_hidma: comment platform_driver_register call

[Sinan Kaya]

On 5/3/2021 7:57 AM, Greg Kroah-Hartman wrote:
> From: Phillip Potter <phil@philpotter.co.uk>
> 
> 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.
> 
> Cc: Sinan Kaya <okaya@kernel.org>
> Cc: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/dma/qcom/hidma_mgmt.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/dma/qcom/hidma_mgmt.c b/drivers/dma/qcom/hidma_mgmt.c
> index fe87b01f7a4e..62026607f3f8 100644
> --- a/drivers/dma/qcom/hidma_mgmt.c
> +++ b/drivers/dma/qcom/hidma_mgmt.c
> @@ -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;
> 

Acked-By: Sinan Kaya <okaya@kernel.org>

Re: [PATCH 51/69] dmaengine: qcom_hidma: comment platform_driver_register call

[Vinod Koul]

On 03-05-21, 13:57, Greg Kroah-Hartman wrote:
> From: Phillip Potter <phil@philpotter.co.uk>
> 
> 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.

Acked-By: Vinod Koul <vkoul@kernel.org>

-- 
~Vinod

[PATCH 52/69] Revert "libertas: add checks for the return value of sysfs_create_group"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Kalle Valo <kvalo@codeaurora.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/wireless/marvell/libertas/mesh.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/mesh.c b/drivers/net/wireless/marvell/libertas/mesh.c
index f5b78257d551..c611e6668b21 100644
--- a/drivers/net/wireless/marvell/libertas/mesh.c
+++ b/drivers/net/wireless/marvell/libertas/mesh.c
@@ -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

[PATCH 53/69] libertas: register sysfs groups properly

[Greg Kroah-Hartman]

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.

Cc: Kalle Valo <kvalo@codeaurora.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/wireless/marvell/libertas/mesh.c | 28 +++-----------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/mesh.c b/drivers/net/wireless/marvell/libertas/mesh.c
index c611e6668b21..c68814841583 100644
--- a/drivers/net/wireless/marvell/libertas/mesh.c
+++ b/drivers/net/wireless/marvell/libertas/mesh.c
@@ -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

[PATCH 54/69] Revert "ASoC: rt5645: fix a NULL pointer dereference"

[Greg Kroah-Hartman]

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.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/soc/codecs/rt5645.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index 9408ee63cb26..7cb90975009a 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -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

[PATCH 55/69] ASoC: rt5645: add error checking to rt5645_probe function

[Greg Kroah-Hartman]

From: Phillip Potter <phil@philpotter.co.uk>

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.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/soc/codecs/rt5645.c | 48 +++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index 7cb90975009a..438fa18bcb55 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -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

Re: [PATCH 55/69] ASoC: rt5645: add error checking to rt5645_probe function

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]

On Mon, May 03, 2021 at 01:57:22PM +0200, Greg Kroah-Hartman wrote:
> From: Phillip Potter <phil@philpotter.co.uk>
> 
> 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

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.

> +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;

This comment is not accurate, rt5645_remove() just resets the
hardware - it's not going to clean up anything to do with any of
the branches to error you've got above.  The core *will* clean up
any routes and widgets that are added, but it doesn't do it by
calling remove() and people shouldn't add code in their remove
functions which does so.

Also I'm guessing this was done purely through inspection rather
than the code having been tested?  If there was a problem seen at
runtime this isn't fixing it, TBH I'm more than a little dubious
about applying this untested - it's really random if things check
these errors since they're basically static checks that we're not
smart enough to do at compile time and the core is pretty loud
when they hit.  I occasionally wonder about just removing the
return codes, I think more callers don't have the checks than do
(certainly in the case of _force_enable() where I was surprised
to find any callers that do), but never got round to it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 55/69] ASoC: rt5645: add error checking to rt5645_probe function

[Phillip Potter]

On Tue, May 25, 2021 at 10:38:45PM +0100, Mark Brown wrote:
> On Mon, May 03, 2021 at 01:57:22PM +0200, Greg Kroah-Hartman wrote:
> > From: Phillip Potter <phil@philpotter.co.uk>
> > 
> > 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
> 
> 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.
> 

Dear Mark,

This patch was submitted to a closed mentoring group as part of the
University of Minnesota reversion/checking process. I was not
responsible for the final send out to the public mailing lists etc. as
the patches were collated first and sent out together en masse.

> > +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;
> 
> This comment is not accurate, rt5645_remove() just resets the
> hardware - it's not going to clean up anything to do with any of
> the branches to error you've got above.  The core *will* clean up
> any routes and widgets that are added, but it doesn't do it by
> calling remove() and people shouldn't add code in their remove
> functions which does so.

My comment was adjusted after submission for brevity's sake. This was
what I originally wrote:
 /*
  * All of the above is cleaned up when we return an error here, as
  * the caller will notice the error and invoke rt5645_remove and
  * other cleanup routines, as it does for the snd_soc_dapm_* calls
  * above as well.
  */
Happy to resubmit/rewrite as needed? Based on what you've written
though it may be better to drop the patch?

> 
> Also I'm guessing this was done purely through inspection rather
> than the code having been tested?  If there was a problem seen at
> runtime this isn't fixing it, TBH I'm more than a little dubious
> about applying this untested - it's really random if things check
> these errors since they're basically static checks that we're not
> smart enough to do at compile time and the core is pretty loud
> when they hit.  I occasionally wonder about just removing the
> return codes, I think more callers don't have the checks than do
> (certainly in the case of _force_enable() where I was surprised
> to find any callers that do), but never got round to it.

Yes, that's correct - I did not test this directly other than making
sure it builds, as I don't have this hardware to test with. 

Regards,
Phil

Re: [PATCH 55/69] ASoC: rt5645: add error checking to rt5645_probe function

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]

On Tue, May 25, 2021 at 11:02:16PM +0100, Phillip Potter wrote:
> On Tue, May 25, 2021 at 10:38:45PM +0100, Mark Brown wrote:

> > 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

> This patch was submitted to a closed mentoring group as part of the
> University of Minnesota reversion/checking process. I was not
> responsible for the final send out to the public mailing lists etc. as
> the patches were collated first and sent out together en masse.

OK, this is really unfortunate.

> > This comment is not accurate, rt5645_remove() just resets the
> > hardware - it's not going to clean up anything to do with any of
> > the branches to error you've got above.  The core *will* clean up

> My comment was adjusted after submission for brevity's sake. This was
> what I originally wrote:
>  /*
>   * All of the above is cleaned up when we return an error here, as
>   * the caller will notice the error and invoke rt5645_remove and
>   * other cleanup routines, as it does for the snd_soc_dapm_* calls
>   * above as well.
>   */
> Happy to resubmit/rewrite as needed? Based on what you've written
> though it may be better to drop the patch?

That is a lot better yes, it accurately reflects what was going
on - the review definitely wasn't helping here.

> > Also I'm guessing this was done purely through inspection rather
> > than the code having been tested?  If there was a problem seen at
> > runtime this isn't fixing it, TBH I'm more than a little dubious

> Yes, that's correct - I did not test this directly other than making
> sure it builds, as I don't have this hardware to test with. 

OK, in that case it's going to be safer to just drop the change,
it's probably not going to cause any actual problems but it's
certainly not something that should go in as a hurried fix.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 55/69] ASoC: rt5645: add error checking to rt5645_probe function

[Phillip Potter]

On Thu, May 27, 2021 at 05:31:57PM +0100, Mark Brown wrote:
> On Tue, May 25, 2021 at 11:02:16PM +0100, Phillip Potter wrote:
> > On Tue, May 25, 2021 at 10:38:45PM +0100, Mark Brown wrote:
> 
> > > 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
> 
> > This patch was submitted to a closed mentoring group as part of the
> > University of Minnesota reversion/checking process. I was not
> > responsible for the final send out to the public mailing lists etc. as
> > the patches were collated first and sent out together en masse.
> 
> OK, this is really unfortunate.
> 
> > > This comment is not accurate, rt5645_remove() just resets the
> > > hardware - it's not going to clean up anything to do with any of
> > > the branches to error you've got above.  The core *will* clean up
> 
> > My comment was adjusted after submission for brevity's sake. This was
> > what I originally wrote:
> >  /*
> >   * All of the above is cleaned up when we return an error here, as
> >   * the caller will notice the error and invoke rt5645_remove and
> >   * other cleanup routines, as it does for the snd_soc_dapm_* calls
> >   * above as well.
> >   */
> > Happy to resubmit/rewrite as needed? Based on what you've written
> > though it may be better to drop the patch?
> 
> That is a lot better yes, it accurately reflects what was going
> on - the review definitely wasn't helping here.
> 
> > > Also I'm guessing this was done purely through inspection rather
> > > than the code having been tested?  If there was a problem seen at
> > > runtime this isn't fixing it, TBH I'm more than a little dubious
> 
> > Yes, that's correct - I did not test this directly other than making
> > sure it builds, as I don't have this hardware to test with. 
> 
> OK, in that case it's going to be safer to just drop the change,
> it's probably not going to cause any actual problems but it's
> certainly not something that should go in as a hurried fix.

Dear Mark,

Thank you, I will not resubmit with the new comment in that case. Have a
great weekend.

Regards,
Phil

[PATCH 56/69] Revert "ASoC: cs43130: fix a NULL pointer dereference"

[Greg Kroah-Hartman]

This reverts commit a2be42f18d409213bb7e7a736e3ef6ba005115bb.

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 here is not correct, sysfs files that were created
are not unwound.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/soc/codecs/cs43130.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c
index 80bc7c10ed75..c2b6f0ae6d57 100644
--- a/sound/soc/codecs/cs43130.c
+++ b/sound/soc/codecs/cs43130.c
@@ -2319,8 +2319,6 @@ static int cs43130_probe(struct snd_soc_component *component)
 			return ret;
 
 		cs43130->wq = create_singlethread_workqueue("cs43130_hp");
-		if (!cs43130->wq)
-			return -ENOMEM;
 		INIT_WORK(&cs43130->work, cs43130_imp_meas);
 	}
 
-- 
2.31.1

[PATCH 57/69] ASoC: cs43130: handle errors in cs43130_probe() properly

[Greg Kroah-Hartman]

cs43130_probe() does not do any valid error checking of things it
initializes, OR what it does, it does not unwind properly if there are
errors.

Fix this up by moving the sysfs files to an attribute group so the
driver core will correctly add/remove them all at once and handle errors
with them, and correctly check for creating a new workqueue and
unwinding if that fails.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/soc/codecs/cs43130.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c
index c2b6f0ae6d57..80cd3ea0c157 100644
--- a/sound/soc/codecs/cs43130.c
+++ b/sound/soc/codecs/cs43130.c
@@ -1735,6 +1735,14 @@ static DEVICE_ATTR(hpload_dc_r, 0444, cs43130_show_dc_r, NULL);
 static DEVICE_ATTR(hpload_ac_l, 0444, cs43130_show_ac_l, NULL);
 static DEVICE_ATTR(hpload_ac_r, 0444, cs43130_show_ac_r, NULL);
 
+static struct attribute *hpload_attrs[] = {
+	&dev_attr_hpload_dc_l.attr,
+	&dev_attr_hpload_dc_r.attr,
+	&dev_attr_hpload_ac_l.attr,
+	&dev_attr_hpload_ac_r.attr,
+};
+ATTRIBUTE_GROUPS(hpload);
+
 static struct reg_sequence hp_en_cal_seq[] = {
 	{CS43130_INT_MASK_4, CS43130_INT_MASK_ALL},
 	{CS43130_HP_MEAS_LOAD_1, 0},
@@ -2302,23 +2310,15 @@ static int cs43130_probe(struct snd_soc_component *component)
 
 	cs43130->hpload_done = false;
 	if (cs43130->dc_meas) {
-		ret = device_create_file(component->dev, &dev_attr_hpload_dc_l);
-		if (ret < 0)
-			return ret;
-
-		ret = device_create_file(component->dev, &dev_attr_hpload_dc_r);
-		if (ret < 0)
-			return ret;
-
-		ret = device_create_file(component->dev, &dev_attr_hpload_ac_l);
-		if (ret < 0)
-			return ret;
-
-		ret = device_create_file(component->dev, &dev_attr_hpload_ac_r);
-		if (ret < 0)
+		ret = sysfs_create_groups(&component->dev->kobj, hpload_groups);
+		if (ret)
 			return ret;
 
 		cs43130->wq = create_singlethread_workqueue("cs43130_hp");
+		if (!cs43130->wq) {
+			sysfs_remove_groups(&component->dev->kobj, hpload_groups);
+			return -ENOMEM;
+		}
 		INIT_WORK(&cs43130->work, cs43130_imp_meas);
 	}
 
-- 
2.31.1

[PATCH 58/69] Revert "media: dvb: Add check on sp8870_readreg"

[Greg Kroah-Hartman]

This reverts commit 467a37fba93f2b4fe3ab597ff6a517b22b566882.

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 properly checking for an error at all, so if a
read succeeds from this device, it will error out.

Cc: Aditya Pakki <pakki001@umn.edu>
Cc: Sean Young <sean@mess.org>
Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/media/dvb-frontends/sp8870.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/dvb-frontends/sp8870.c b/drivers/media/dvb-frontends/sp8870.c
index 655db8272268..ee893a2f2261 100644
--- a/drivers/media/dvb-frontends/sp8870.c
+++ b/drivers/media/dvb-frontends/sp8870.c
@@ -280,9 +280,7 @@ static int sp8870_set_frontend_parameters(struct dvb_frontend *fe)
 	sp8870_writereg(state, 0xc05, reg0xc05);
 
 	// read status reg in order to clear pending irqs
-	err = sp8870_readreg(state, 0x200);
-	if (err)
-		return err;
+	sp8870_readreg(state, 0x200);
 
 	// system controller start
 	sp8870_microcontroller_start(state);
-- 
2.31.1

[PATCH 59/69] media: dvb: Add check on sp8870_readreg return

[Greg Kroah-Hartman]

From: Alaa Emad <alaaemadhossney.ae@gmail.com>

The function sp8870_readreg returns a negative value when i2c_transfer
fails so properly check for this and return the error if it happens.

Cc: Sean Young <sean@mess.org>
Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Signed-off-by: Alaa Emad <alaaemadhossney.ae@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/media/dvb-frontends/sp8870.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/sp8870.c b/drivers/media/dvb-frontends/sp8870.c
index ee893a2f2261..9767159aeb9b 100644
--- a/drivers/media/dvb-frontends/sp8870.c
+++ b/drivers/media/dvb-frontends/sp8870.c
@@ -280,7 +280,9 @@ static int sp8870_set_frontend_parameters(struct dvb_frontend *fe)
 	sp8870_writereg(state, 0xc05, reg0xc05);
 
 	// read status reg in order to clear pending irqs
-	sp8870_readreg(state, 0x200);
+	err = sp8870_readreg(state, 0x200);
+	if (err < 0)
+		return err;
 
 	// system controller start
 	sp8870_microcontroller_start(state);
-- 
2.31.1

[PATCH 60/69] Revert "media: gspca: mt9m111: Check write_bridge for timeout"

[Greg Kroah-Hartman]

This reverts commit 656025850074f5c1ba2e05be37bda57ba2b8d491.

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.

Different error values should never be "OR" together and expect anything
sane to come out of the result.

Cc: Aditya Pakki <pakki001@umn.edu>
Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/media/usb/gspca/m5602/m5602_mt9m111.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/gspca/m5602/m5602_mt9m111.c b/drivers/media/usb/gspca/m5602/m5602_mt9m111.c
index bfa3b381d8a2..50481dc928d0 100644
--- a/drivers/media/usb/gspca/m5602/m5602_mt9m111.c
+++ b/drivers/media/usb/gspca/m5602/m5602_mt9m111.c
@@ -195,7 +195,7 @@ static const struct v4l2_ctrl_config mt9m111_greenbal_cfg = {
 int mt9m111_probe(struct sd *sd)
 {
 	u8 data[2] = {0x00, 0x00};
-	int i, rc = 0;
+	int i;
 	struct gspca_dev *gspca_dev = (struct gspca_dev *)sd;
 
 	if (force_sensor) {
@@ -213,18 +213,16 @@ int mt9m111_probe(struct sd *sd)
 	/* Do the preinit */
 	for (i = 0; i < ARRAY_SIZE(preinit_mt9m111); i++) {
 		if (preinit_mt9m111[i][0] == BRIDGE) {
-			rc |= m5602_write_bridge(sd,
+			m5602_write_bridge(sd,
 				preinit_mt9m111[i][1],
 				preinit_mt9m111[i][2]);
 		} else {
 			data[0] = preinit_mt9m111[i][2];
 			data[1] = preinit_mt9m111[i][3];
-			rc |= m5602_write_sensor(sd,
+			m5602_write_sensor(sd,
 				preinit_mt9m111[i][1], data, 2);
 		}
 	}
-	if (rc < 0)
-		return rc;
 
 	if (m5602_read_sensor(sd, MT9M111_SC_CHIPVER, data, 2))
 		return -ENODEV;
-- 
2.31.1

[PATCH 61/69] media: gspca: mt9m111: Check write_bridge for timeout

[Greg Kroah-Hartman]

From: Alaa Emad <alaaemadhossney.ae@gmail.com>

If m5602_write_bridge times out, it will return a negative error value.
So properly check for this and handle the error correctly instead of
just ignoring it.

Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Signed-off-by: Alaa Emad <alaaemadhossney.ae@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/media/usb/gspca/m5602/m5602_mt9m111.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/gspca/m5602/m5602_mt9m111.c b/drivers/media/usb/gspca/m5602/m5602_mt9m111.c
index 50481dc928d0..bf1af6ed9131 100644
--- a/drivers/media/usb/gspca/m5602/m5602_mt9m111.c
+++ b/drivers/media/usb/gspca/m5602/m5602_mt9m111.c
@@ -195,7 +195,7 @@ static const struct v4l2_ctrl_config mt9m111_greenbal_cfg = {
 int mt9m111_probe(struct sd *sd)
 {
 	u8 data[2] = {0x00, 0x00};
-	int i;
+	int i, err;
 	struct gspca_dev *gspca_dev = (struct gspca_dev *)sd;
 
 	if (force_sensor) {
@@ -213,15 +213,17 @@ int mt9m111_probe(struct sd *sd)
 	/* Do the preinit */
 	for (i = 0; i < ARRAY_SIZE(preinit_mt9m111); i++) {
 		if (preinit_mt9m111[i][0] == BRIDGE) {
-			m5602_write_bridge(sd,
-				preinit_mt9m111[i][1],
-				preinit_mt9m111[i][2]);
+			err = m5602_write_bridge(sd,
+					preinit_mt9m111[i][1],
+					preinit_mt9m111[i][2]);
 		} else {
 			data[0] = preinit_mt9m111[i][2];
 			data[1] = preinit_mt9m111[i][3];
-			m5602_write_sensor(sd,
-				preinit_mt9m111[i][1], data, 2);
+			err = m5602_write_sensor(sd,
+					preinit_mt9m111[i][1], data, 2);
 		}
+		if (err < 0)
+			return err;
 	}
 
 	if (m5602_read_sensor(sd, MT9M111_SC_CHIPVER, data, 2))
-- 
2.31.1

[PATCH 62/69] Revert "media: gspca: Check the return value of write_bridge for timeout"

[Greg Kroah-Hartman]

This reverts commit a21a0eb56b4e8fe4a330243af8030f890cde2283.

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.

Different error values should never be "OR" together and expect anything
sane to come out of the result.

Cc: Aditya Pakki <pakki001@umn.edu>
Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/media/usb/gspca/m5602/m5602_po1030.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/gspca/m5602/m5602_po1030.c b/drivers/media/usb/gspca/m5602/m5602_po1030.c
index d680b777f097..7bdbb8065146 100644
--- a/drivers/media/usb/gspca/m5602/m5602_po1030.c
+++ b/drivers/media/usb/gspca/m5602/m5602_po1030.c
@@ -154,7 +154,6 @@ static const struct v4l2_ctrl_config po1030_greenbal_cfg = {
 
 int po1030_probe(struct sd *sd)
 {
-	int rc = 0;
 	u8 dev_id_h = 0, i;
 	struct gspca_dev *gspca_dev = (struct gspca_dev *)sd;
 
@@ -174,14 +173,11 @@ int po1030_probe(struct sd *sd)
 	for (i = 0; i < ARRAY_SIZE(preinit_po1030); i++) {
 		u8 data = preinit_po1030[i][2];
 		if (preinit_po1030[i][0] == SENSOR)
-			rc |= m5602_write_sensor(sd,
+			m5602_write_sensor(sd,
 				preinit_po1030[i][1], &data, 1);
 		else
-			rc |= m5602_write_bridge(sd, preinit_po1030[i][1],
-						data);
+			m5602_write_bridge(sd, preinit_po1030[i][1], data);
 	}
-	if (rc < 0)
-		return rc;
 
 	if (m5602_read_sensor(sd, PO1030_DEVID_H, &dev_id_h, 1))
 		return -ENODEV;
-- 
2.31.1

[PATCH 63/69] media: gspca: properly check for errors in po1030_probe()

[Greg Kroah-Hartman]

If m5602_write_sensor() or m5602_write_bridge() fail, do not continue to
initialize the device but return the error to the calling funtion.

Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/media/usb/gspca/m5602/m5602_po1030.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/gspca/m5602/m5602_po1030.c b/drivers/media/usb/gspca/m5602/m5602_po1030.c
index 7bdbb8065146..8fd99ceee4b6 100644
--- a/drivers/media/usb/gspca/m5602/m5602_po1030.c
+++ b/drivers/media/usb/gspca/m5602/m5602_po1030.c
@@ -155,6 +155,7 @@ static const struct v4l2_ctrl_config po1030_greenbal_cfg = {
 int po1030_probe(struct sd *sd)
 {
 	u8 dev_id_h = 0, i;
+	int err;
 	struct gspca_dev *gspca_dev = (struct gspca_dev *)sd;
 
 	if (force_sensor) {
@@ -173,10 +174,13 @@ int po1030_probe(struct sd *sd)
 	for (i = 0; i < ARRAY_SIZE(preinit_po1030); i++) {
 		u8 data = preinit_po1030[i][2];
 		if (preinit_po1030[i][0] == SENSOR)
-			m5602_write_sensor(sd,
-				preinit_po1030[i][1], &data, 1);
+			err = m5602_write_sensor(sd, preinit_po1030[i][1],
+						 &data, 1);
 		else
-			m5602_write_bridge(sd, preinit_po1030[i][1], data);
+			err = m5602_write_bridge(sd, preinit_po1030[i][1],
+						 data);
+		if (err < 0)
+			return err;
 	}
 
 	if (m5602_read_sensor(sd, PO1030_DEVID_H, &dev_id_h, 1))
-- 
2.31.1

[PATCH 64/69] Revert "net: liquidio: fix a NULL pointer dereference"

[Greg Kroah-Hartman]

This reverts commit fe543b2f174f34a7a751aa08b334fe6b105c4569.

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 the original commit does keep the immediate "NULL dereference"
from happening, it does not properly propagate the error back to the
callers, AND it does not fix this same identical issue in the
drivers/net/ethernet/cavium/liquidio/lio_vf_main.c for some reason.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/ethernet/cavium/liquidio/lio_main.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
index 7c5af4beedc6..6fa570068648 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
@@ -1166,11 +1166,6 @@ static void send_rx_ctrl_cmd(struct lio *lio, int start_stop)
 	sc = (struct octeon_soft_command *)
 		octeon_alloc_soft_command(oct, OCTNET_CMD_SIZE,
 					  16, 0);
-	if (!sc) {
-		netif_info(lio, rx_err, lio->netdev,
-			   "Failed to allocate octeon_soft_command\n");
-		return;
-	}
 
 	ncmd = (union octnet_cmd *)sc->virtdptr;
 
-- 
2.31.1

[PATCH 65/69] net: liquidio: Add missing null pointer checks

[Greg Kroah-Hartman]

From: Tom Seewald <tseewald@gmail.com>

The functions send_rx_ctrl_cmd() in both liquidio/lio_main.c and
liquidio/lio_vf_main.c do not check if the call to
octeon_alloc_soft_command() fails and returns a null pointer. Both
functions also return void so errors are not propagated back to the
caller.

Fix these issues by updating both instances of send_rx_ctrl_cmd() to
return an integer rather than void, and have them return -ENOMEM if an
allocation failure occurs. Also update all callers of send_rx_ctrl_cmd()
so that they now check the return value.

Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Tom Seewald <tseewald@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 .../net/ethernet/cavium/liquidio/lio_main.c   | 28 +++++++++++++------
 .../ethernet/cavium/liquidio/lio_vf_main.c    | 27 +++++++++++++-----
 2 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
index 6fa570068648..591229b96257 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
@@ -1153,7 +1153,7 @@ static void octeon_destroy_resources(struct octeon_device *oct)
  * @lio: per-network private data
  * @start_stop: whether to start or stop
  */
-static void send_rx_ctrl_cmd(struct lio *lio, int start_stop)
+static int send_rx_ctrl_cmd(struct lio *lio, int start_stop)
 {
 	struct octeon_soft_command *sc;
 	union octnet_cmd *ncmd;
@@ -1161,11 +1161,16 @@ static void send_rx_ctrl_cmd(struct lio *lio, int start_stop)
 	int retval;
 
 	if (oct->props[lio->ifidx].rx_on == start_stop)
-		return;
+		return 0;
 
 	sc = (struct octeon_soft_command *)
 		octeon_alloc_soft_command(oct, OCTNET_CMD_SIZE,
 					  16, 0);
+	if (!sc) {
+		netif_info(lio, rx_err, lio->netdev,
+			   "Failed to allocate octeon_soft_command struct\n");
+		return -ENOMEM;
+	}
 
 	ncmd = (union octnet_cmd *)sc->virtdptr;
 
@@ -1187,18 +1192,19 @@ static void send_rx_ctrl_cmd(struct lio *lio, int start_stop)
 	if (retval == IQ_SEND_FAILED) {
 		netif_info(lio, rx_err, lio->netdev, "Failed to send RX Control message\n");
 		octeon_free_soft_command(oct, sc);
-		return;
 	} else {
 		/* Sleep on a wait queue till the cond flag indicates that the
 		 * response arrived or timed-out.
 		 */
 		retval = wait_for_sc_completion_timeout(oct, sc, 0);
 		if (retval)
-			return;
+			return retval;
 
 		oct->props[lio->ifidx].rx_on = start_stop;
 		WRITE_ONCE(sc->caller_is_done, true);
 	}
+
+	return retval;
 }
 
 /**
@@ -1773,6 +1779,7 @@ static int liquidio_open(struct net_device *netdev)
 	struct octeon_device_priv *oct_priv =
 		(struct octeon_device_priv *)oct->priv;
 	struct napi_struct *napi, *n;
+	int ret = 0;
 
 	if (oct->props[lio->ifidx].napi_enabled == 0) {
 		tasklet_disable(&oct_priv->droq_tasklet);
@@ -1808,7 +1815,9 @@ static int liquidio_open(struct net_device *netdev)
 	netif_info(lio, ifup, lio->netdev, "Interface Open, ready for traffic\n");
 
 	/* tell Octeon to start forwarding packets to host */
-	send_rx_ctrl_cmd(lio, 1);
+	ret = send_rx_ctrl_cmd(lio, 1);
+	if (ret)
+		return ret;
 
 	/* start periodical statistics fetch */
 	INIT_DELAYED_WORK(&lio->stats_wk.work, lio_fetch_stats);
@@ -1819,7 +1828,7 @@ static int liquidio_open(struct net_device *netdev)
 	dev_info(&oct->pci_dev->dev, "%s interface is opened\n",
 		 netdev->name);
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -1833,6 +1842,7 @@ static int liquidio_stop(struct net_device *netdev)
 	struct octeon_device_priv *oct_priv =
 		(struct octeon_device_priv *)oct->priv;
 	struct napi_struct *napi, *n;
+	int ret = 0;
 
 	ifstate_reset(lio, LIO_IFSTATE_RUNNING);
 
@@ -1849,7 +1859,9 @@ static int liquidio_stop(struct net_device *netdev)
 	lio->link_changes++;
 
 	/* Tell Octeon that nic interface is down. */
-	send_rx_ctrl_cmd(lio, 0);
+	ret = send_rx_ctrl_cmd(lio, 0);
+	if (ret)
+		return ret;
 
 	if (OCTEON_CN23XX_PF(oct)) {
 		if (!oct->msix_on)
@@ -1884,7 +1896,7 @@ static int liquidio_stop(struct net_device *netdev)
 
 	dev_info(&oct->pci_dev->dev, "%s interface is stopped\n", netdev->name);
 
-	return 0;
+	return ret;
 }
 
 /**
diff --git a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
index 516f166ceff8..ffddb3126a32 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
@@ -595,7 +595,7 @@ static void octeon_destroy_resources(struct octeon_device *oct)
  * @lio: per-network private data
  * @start_stop: whether to start or stop
  */
-static void send_rx_ctrl_cmd(struct lio *lio, int start_stop)
+static int send_rx_ctrl_cmd(struct lio *lio, int start_stop)
 {
 	struct octeon_device *oct = (struct octeon_device *)lio->oct_dev;
 	struct octeon_soft_command *sc;
@@ -603,11 +603,16 @@ static void send_rx_ctrl_cmd(struct lio *lio, int start_stop)
 	int retval;
 
 	if (oct->props[lio->ifidx].rx_on == start_stop)
-		return;
+		return 0;
 
 	sc = (struct octeon_soft_command *)
 		octeon_alloc_soft_command(oct, OCTNET_CMD_SIZE,
 					  16, 0);
+	if (!sc) {
+		netif_info(lio, rx_err, lio->netdev,
+			   "Failed to allocate octeon_soft_command struct\n");
+		return -ENOMEM;
+	}
 
 	ncmd = (union octnet_cmd *)sc->virtdptr;
 
@@ -635,11 +640,13 @@ static void send_rx_ctrl_cmd(struct lio *lio, int start_stop)
 		 */
 		retval = wait_for_sc_completion_timeout(oct, sc, 0);
 		if (retval)
-			return;
+			return retval;
 
 		oct->props[lio->ifidx].rx_on = start_stop;
 		WRITE_ONCE(sc->caller_is_done, true);
 	}
+
+	return retval;
 }
 
 /**
@@ -906,6 +913,7 @@ static int liquidio_open(struct net_device *netdev)
 	struct octeon_device_priv *oct_priv =
 		(struct octeon_device_priv *)oct->priv;
 	struct napi_struct *napi, *n;
+	int ret = 0;
 
 	if (!oct->props[lio->ifidx].napi_enabled) {
 		tasklet_disable(&oct_priv->droq_tasklet);
@@ -932,11 +940,13 @@ static int liquidio_open(struct net_device *netdev)
 					(LIQUIDIO_NDEV_STATS_POLL_TIME_MS));
 
 	/* tell Octeon to start forwarding packets to host */
-	send_rx_ctrl_cmd(lio, 1);
+	ret = send_rx_ctrl_cmd(lio, 1);
+	if (ret)
+		return ret;
 
 	dev_info(&oct->pci_dev->dev, "%s interface is opened\n", netdev->name);
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -950,9 +960,12 @@ static int liquidio_stop(struct net_device *netdev)
 	struct octeon_device_priv *oct_priv =
 		(struct octeon_device_priv *)oct->priv;
 	struct napi_struct *napi, *n;
+	int ret = 0;
 
 	/* tell Octeon to stop forwarding packets to host */
-	send_rx_ctrl_cmd(lio, 0);
+	ret = send_rx_ctrl_cmd(lio, 0);
+	if (ret)
+		return ret;
 
 	netif_info(lio, ifdown, lio->netdev, "Stopping interface!\n");
 	/* Inform that netif carrier is down */
@@ -986,7 +999,7 @@ static int liquidio_stop(struct net_device *netdev)
 
 	dev_info(&oct->pci_dev->dev, "%s interface is stopped\n", netdev->name);
 
-	return 0;
+	return ret;
 }
 
 /**
-- 
2.31.1

[PATCH 66/69] Revert "video: imsttfb: fix potential NULL pointer dereferences"

[Greg Kroah-Hartman]

This reverts commit 1d84353d205a953e2381044953b7fa31c8c9702d.

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 here, while technically correct, did not fully
handle all of the reported issues that the commit stated it was fixing,
so revert it until it can be "fixed" fully.

Note, ioremap() probably will never fail for old hardware like this, and
if anyone actually used this hardware (a PowerMac era PCI display card),
they would not be using fbdev anymore.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Aditya Pakki <pakki001@umn.edu>
Cc: Finn Thain <fthain@telegraphics.com.au>
Cc: Rob Herring <robh@kernel.org>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Fixes: 1d84353d205a ("video: imsttfb: fix potential NULL pointer dereferences")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/imsttfb.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index 3ac053b88495..e04411701ec8 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -1512,11 +1512,6 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	info->fix.smem_start = addr;
 	info->screen_base = (__u8 *)ioremap(addr, par->ramdac == IBM ?
 					    0x400000 : 0x800000);
-	if (!info->screen_base) {
-		release_mem_region(addr, size);
-		framebuffer_release(info);
-		return -ENOMEM;
-	}
 	info->fix.mmio_start = addr + 0x800000;
 	par->dc_regs = ioremap(addr + 0x800000, 0x1000);
 	par->cmap_regs_phys = addr + 0x840000;
-- 
2.31.1

Re: [PATCH 66/69] Revert "video: imsttfb: fix potential NULL pointer dereferences"

[Rob Herring]

On Mon, May 3, 2021 at 7:01 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> This reverts commit 1d84353d205a953e2381044953b7fa31c8c9702d.
>
> 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 here, while technically correct, did not fully
> handle all of the reported issues that the commit stated it was fixing,
> so revert it until it can be "fixed" fully.
>
> Note, ioremap() probably will never fail for old hardware like this, and
> if anyone actually used this hardware (a PowerMac era PCI display card),
> they would not be using fbdev anymore.
>
> Cc: Kangjie Lu <kjlu@umn.edu>
> Cc: Aditya Pakki <pakki001@umn.edu>
> Cc: Finn Thain <fthain@telegraphics.com.au>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Fixes: 1d84353d205a ("video: imsttfb: fix potential NULL pointer dereferences")
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/video/fbdev/imsttfb.c | 5 -----
>  1 file changed, 5 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 67/69] video: imsttfb: check for ioremap() failures

[Greg Kroah-Hartman]

We should check if ioremap() were to somehow fail in imsttfb_probe() and
handle the unwinding of the resources allocated here properly.

Ideally if anyone cares about this driver (it's for a PowerMac era PCI
display card), they wouldn't even be using fbdev anymore.  Or the devm_*
apis could be used, but that's just extra work for diminishing
returns...

Cc: Finn Thain <fthain@telegraphics.com.au>
Cc: Rob Herring <robh@kernel.org>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/imsttfb.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index e04411701ec8..16f272a50811 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -1469,6 +1469,7 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	struct imstt_par *par;
 	struct fb_info *info;
 	struct device_node *dp;
+	int ret = -ENOMEM;
 	
 	dp = pci_device_to_OF_node(pdev);
 	if(dp)
@@ -1504,23 +1505,37 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		default:
 			printk(KERN_INFO "imsttfb: Device 0x%x unknown, "
 					 "contact maintainer.\n", pdev->device);
-			release_mem_region(addr, size);
-			framebuffer_release(info);
-			return -ENODEV;
+			ret = -ENODEV;
+			goto error;
 	}
 
 	info->fix.smem_start = addr;
 	info->screen_base = (__u8 *)ioremap(addr, par->ramdac == IBM ?
 					    0x400000 : 0x800000);
+	if (!info->screen_base)
+		goto error;
 	info->fix.mmio_start = addr + 0x800000;
 	par->dc_regs = ioremap(addr + 0x800000, 0x1000);
+	if (!par->dc_regs)
+		goto error;
 	par->cmap_regs_phys = addr + 0x840000;
 	par->cmap_regs = (__u8 *)ioremap(addr + 0x840000, 0x1000);
+	if (!par->cmap_regs)
+		goto error;
 	info->pseudo_palette = par->palette;
 	init_imstt(info);
 
 	pci_set_drvdata(pdev, info);
 	return 0;
+
+error:
+	if (par->dc_regs)
+		iounmap(par->dc_regs);
+	if (info->screen_base)
+		iounmap(info->screen_base);
+	release_mem_region(addr, size);
+	framebuffer_release(info);
+	return ret;
 }
 
 static void imsttfb_remove(struct pci_dev *pdev)
-- 
2.31.1

Re: [PATCH 67/69] video: imsttfb: check for ioremap() failures

[Rob Herring]

On Mon, May 3, 2021 at 7:01 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> We should check if ioremap() were to somehow fail in imsttfb_probe() and
> handle the unwinding of the resources allocated here properly.
>
> Ideally if anyone cares about this driver (it's for a PowerMac era PCI
> display card), they wouldn't even be using fbdev anymore.  Or the devm_*
> apis could be used, but that's just extra work for diminishing
> returns...
>
> Cc: Finn Thain <fthain@telegraphics.com.au>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/video/fbdev/imsttfb.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 68/69] Revert "brcmfmac: add a check for the status of usb_register"

[Greg Kroah-Hartman]

This reverts commit 42daad3343be4a4e1ee03e30a5f5cc731dadfef5.

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 here did nothing to actually help if usb_register()
failed, so it gives a "false sense of security" when there is none.  The
correct solution is to correctly unwind from this error.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Kalle Valo <kvalo@codeaurora.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 586f4dfc638b..d2a803fc8ac6 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1586,10 +1586,6 @@ void brcmf_usb_exit(void)
 
 void brcmf_usb_register(void)
 {
-	int ret;
-
 	brcmf_dbg(USB, "Enter\n");
-	ret = usb_register(&brcmf_usbdrvr);
-	if (ret)
-		brcmf_err("usb_register failed %d\n", ret);
+	usb_register(&brcmf_usbdrvr);
 }
-- 
2.31.1

[PATCH 69/69] brcmfmac: properly check for bus register errors

[Greg Kroah-Hartman]

The brcmfmac driver ignores any errors on initialization with the
different busses by deferring the initialization to a workqueue and
ignoring all possible errors that might happen.  Fix up all of this by
only allowing the module to load if all bus registering worked properly.

Cc: Kalle Valo <kvalo@codeaurora.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 .../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         |  4 +-
 6 files changed, 41 insertions(+), 46 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index ce8c102df7b3..633d0ab19031 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -1217,13 +1217,9 @@ static struct sdio_driver brcmf_sdmmc_driver = {
 	},
 };
 
-void brcmf_sdio_register(void)
+int brcmf_sdio_register(void)
 {
-	int ret;
-
-	ret = sdio_register_driver(&brcmf_sdmmc_driver);
-	if (ret)
-		brcmf_err("sdio_register_driver failed: %d\n", ret);
+	return sdio_register_driver(&brcmf_sdmmc_driver);
 }
 
 void brcmf_sdio_exit(void)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
index 08f9d47f2e5c..3f5da3bb6aa5 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
@@ -275,11 +275,26 @@ void brcmf_bus_add_txhdrlen(struct device *dev, uint len);
 
 #ifdef CONFIG_BRCMFMAC_SDIO
 void brcmf_sdio_exit(void);
-void brcmf_sdio_register(void);
+int brcmf_sdio_register(void);
+#else
+static inline void brcmf_sdio_exit(void) { }
+static inline int brcmf_sdio_register(void) { return 0; }
 #endif
+
 #ifdef CONFIG_BRCMFMAC_USB
 void brcmf_usb_exit(void);
-void brcmf_usb_register(void);
+int brcmf_usb_register(void);
+#else
+static inline void brcmf_usb_exit(void) { }
+static inline int brcmf_usb_register(void) { return 0; }
+#endif
+
+#ifdef CONFIG_BRCMFMAC_PCIE
+void brcmf_pcie_exit(void);
+int brcmf_pcie_register(void);
+#else
+static inline void brcmf_pcie_exit(void) { }
+static inline int brcmf_pcie_register(void) { return 0; }
 #endif
 
 #endif /* BRCMFMAC_BUS_H */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 838b09b23abf..cee1682d2333 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -1518,40 +1518,34 @@ void brcmf_bus_change_state(struct brcmf_bus *bus, enum brcmf_bus_state state)
 	}
 }
 
-static void brcmf_driver_register(struct work_struct *work)
-{
-#ifdef CONFIG_BRCMFMAC_SDIO
-	brcmf_sdio_register();
-#endif
-#ifdef CONFIG_BRCMFMAC_USB
-	brcmf_usb_register();
-#endif
-#ifdef CONFIG_BRCMFMAC_PCIE
-	brcmf_pcie_register();
-#endif
-}
-static DECLARE_WORK(brcmf_driver_work, brcmf_driver_register);
-
 int __init brcmf_core_init(void)
 {
-	if (!schedule_work(&brcmf_driver_work))
-		return -EBUSY;
+	int err;
 
+	err = brcmf_sdio_register();
+	if (err)
+		return err;
+
+	err = brcmf_usb_register();
+	if (err)
+		goto error_usb_register;
+
+	err = brcmf_pcie_register();
+	if (err)
+		goto error_pcie_register;
 	return 0;
+
+error_pcie_register:
+	brcmf_usb_exit();
+error_usb_register:
+	brcmf_sdio_exit();
+	return err;
 }
 
 void __exit brcmf_core_exit(void)
 {
-	cancel_work_sync(&brcmf_driver_work);
-
-#ifdef CONFIG_BRCMFMAC_SDIO
 	brcmf_sdio_exit();
-#endif
-#ifdef CONFIG_BRCMFMAC_USB
 	brcmf_usb_exit();
-#endif
-#ifdef CONFIG_BRCMFMAC_PCIE
 	brcmf_pcie_exit();
-#endif
 }
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index ad79e3b7e74a..143a705b5cb3 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -2140,15 +2140,10 @@ static struct pci_driver brcmf_pciedrvr = {
 };
 
 
-void brcmf_pcie_register(void)
+int brcmf_pcie_register(void)
 {
-	int err;
-
 	brcmf_dbg(PCIE, "Enter\n");
-	err = pci_register_driver(&brcmf_pciedrvr);
-	if (err)
-		brcmf_err(NULL, "PCIE driver registration failed, err=%d\n",
-			  err);
+	return pci_register_driver(&brcmf_pciedrvr);
 }
 
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.h
index d026401d2001..8e6c227e8315 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.h
@@ -11,9 +11,4 @@ struct brcmf_pciedev {
 	struct brcmf_pciedev_info *devinfo;
 };
 
-
-void brcmf_pcie_exit(void);
-void brcmf_pcie_register(void);
-
-
 #endif /* BRCMFMAC_PCIE_H */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index d2a803fc8ac6..9fb68c2dc7e3 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1584,8 +1584,8 @@ void brcmf_usb_exit(void)
 	usb_deregister(&brcmf_usbdrvr);
 }
 
-void brcmf_usb_register(void)
+int brcmf_usb_register(void)
 {
 	brcmf_dbg(USB, "Enter\n");
-	usb_register(&brcmf_usbdrvr);
+	return usb_register(&brcmf_usbdrvr);
 }
-- 
2.31.1

Re: [PATCH 00/69] "Revert and fix properly" patch series based on umn.edu re-review

[Greg Kroah-Hartman]

On Mon, May 03, 2021 at 01:56:27PM +0200, Greg Kroah-Hartman wrote:
> 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.

These are now merged (minus the ones people asked me to drop) to my
char-misc tree.

thanks,

greg k-h