linux-kernel - Re: [PATCH v2 2/4] mtd: core: protect access to MTD devices while in suspend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Oct 2021 11:13:47 +0200
From:   Boris Brezillon <boris.brezillon@...labora.com>
To:     Sean Nyekjaer <sean@...nix.com>
Cc:     Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        Boris Brezillon <bbrezillon@...nel.org>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] mtd: core: protect access to MTD devices while
 in suspend

On Wed, 20 Oct 2021 10:45:32 +0200
Sean Nyekjaer <sean@...nix.com> wrote:

>  static ssize_t mtd_otp_size(struct mtd_info *mtd, bool is_user)
> @@ -1257,6 +1259,8 @@ int mtd_erase(struct mtd_info *mtd, sruct erase_info *instr)
>  
>  	ledtrig_mtd_activity();
>  
> +	mtd_start_access(master);
> +
>  	if (mtd->flags & MTD_SLC_ON_MLC_EMULATION) {
>  		adjinstr.addr = (loff_t)mtd_div_by_eb(instr->addr, mtd) *
>  				master->erasesize;
> @@ -1278,6 +1282,8 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
>  		}
>  	}
>  
> +	mtd_end_access(master);
> +

The section covered in mtd_erase() is too broad. Put the start/end
calls around the ->_erase() call.

>  	return ret;
>  }



> @@ -1576,7 +1604,6 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
>  		ret_code = mtd_read_oob_std(mtd, from, ops);
>  
>  	mtd_update_ecc_stats(mtd, master, &old_stats);
> -

Unrelated line removal. Please drop this change.

>  	/*
>  	 * In cases where ops->datbuf != NULL, mtd->_read_oob() has semantics
>  	 * similar to mtd->_read(), returning a non-negative integer
> @@ -1615,7 +1642,9 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to,
>  	if (mtd->flags & MTD_SLC_ON_MLC_EMULATION)
>  		return mtd_io_emulated_slc(mtd, to, false, ops);
>  
> -	return mtd_write_oob_std(mtd, to, ops);
> +	ret = mtd_write_oob_std(mtd, to, ops);
> +
> +	return ret;

Ditto, you can keep the 'return mtd_write_oob_std(mtd, to, ops);' here.

>  }
>  EXPORT_SYMBOL_GPL(mtd_write_oob);


> +static inline void mtd_start_access(struct mtd_info *master)
> +{
> +	WARN_ON_ONCE(master != mtd_get_master(master));
> +
> +	/*
> +	 * Don't take the suspend_lock on devices that don't
> +	 * implement the suspend hook. Otherwise, lockdep will
> +	 * complain about nested locks when trying to suspend MTD
> +	 * partitions or MTD devices created by gluebi which are
> +	 * backed by real devices.
> +	 */
> +	if (!master->_suspend)
> +		return;
> +
> +	/*
> +	 * Wait until the device is resumed. Should we have a
> +	 * non-blocking mode here?
> +	 */
> +	while (1) {
> +		down_read(&master->master.suspend_lock);
> +		if (!master->master.suspended)
> +			return;
> +
> +		up_read(&master->master.suspend_lock);
> +		wait_event(master->master.resume_wq, master->master.suspended == 0);

Please keep the tests consistent:

		wait_event(master->master.resume_wq, !master->master.suspended);

> +	}
> +}
> +