linux-kernel - Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities

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:	Mon, 3 Nov 2014 11:08:22 +0800
From:	hujianyang <hujianyang@...wei.com>
To:	Tanya Brokhman <tlinder@...eaurora.org>
CC:	<dedekind1@...il.com>, <ezequiel.garcia@...e-electrons.com>,
	"Richard Weinberger" <richard@....at>,
	open list <linux-kernel@...r.kernel.org>,
	<linux-mtd@...ts.infradead.org>, <linux-arm-msm@...r.kernel.org>,
	"Brian Norris" <computersforpeace@...il.com>,
	David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities

Hi Tanya,

On 2014/11/3 1:14, Tanya Brokhman wrote:
>>
>> This patch add 'struct ubi_device *' for 3 functions. We can get 'ubi_device' from
>> 'ubi_volume'. So I think it's because when we call these functions, the '->ubi'
>> pointer of 'ubi_volume' is not initialized, am I right? This patch use 'vol->ubi'
>> to indicate a 'struct ubi_device *' pointer in some places, I think you are sure
>> of using them.
>>
> 
> 1. for validate_vid_hdr() we don;t have a ubi_volume yet since its part of the attach process so we need struct ubi_device
> 2. for get_exclusive() - you're right. Will fetch dev number from the volume
> 3. for check_av() - you're right. fixed
> 

I'm not sure if 'ubi_volume->ubi' is initialized when we call some kinds of
ubi_err() to print error messages. The reference to a null pointer, we perform
'ubi->ubi_num' in ubi_err(), may crash the kernel. So you should be careful
of these situations not only in above cases but also in other places in your
patch.

>>
>> We have the parameter 'ubi_num' for log in some functions like 'ubi_attach_mtd_dev'
>> before. This patch remove 'ubi_num' in upper changes but keep it in other changes.
>> Do we have a discussed rule to deal with this situation? It's not a big problem~
> 
> I removed it because it made no sense printing it twice:
> "ubi-0: attached mtd-0 (...) to ubi0"?
> so I shortned the message:
> "ubi-0: attched mtd..."
> All the info is still there....
> Same for other messages that printed ubi number.
> 

Could we remove some the 'ubi_num'? I think there are no need to print it
twice in other places, like:

@@ -921,7 +923,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,

 		/* Make sure ubi_num is not busy */
 		if (ubi_devices[ubi_num]) {
-			ubi_err("ubi%d already exists", ubi_num);
+			ubi_err(ubi, "ubi%d already exists", ubi_num);
 			return -EEXIST;
 		}
 	}

and

@@ -970,7 +974,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 	mutex_init(&ubi->fm_mutex);
 	init_rwsem(&ubi->fm_sem);

-	ubi_msg("attaching mtd%d to ubi%d", mtd->index, ubi_num);
+	ubi_msg(ubi, "attaching mtd%d to ubi%d", mtd->index, ubi_num);


>>> @@ -1415,8 +1418,9 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int pnum, int offset, int len)
>>>       return 0;
>>>
>>>   fail:
>>> -    ubi_err("self-check failed for PEB %d", pnum);
>>> -    ubi_msg("hex dump of the %d-%d region", offset, offset + len);
>>> +    ubi_err(ubi, "self-check failed for PEB %d", pnum);
>>> +    ubi_msg(ubi, "hex dump of the %d-%d region",
>>> +         offset, offset + len);
>>>       print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 32, 1, buf, len, 1);
>>>       err = -EINVAL;
>>>   error:
>>
>> Artem, I know you have tried to align the message code in different lines, maybe
>> you can check if you lose this one.
>>
> 
> hmmm... not sure I understand what is wrong here....
> 

Turn

+    ubi_msg(ubi, "hex dump of the %d-%d region",
+         offset, offset + len);

to

+    ubi_msg(ubi, "hex dump of the %d-%d region",
+            offset, offset + len);

Maybe like this. The next line aligns to the message in first line, not a big problem.
By the way, I use space in this example, it's wrong. Tab is right.


Thanks!

Hu


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/