[PATCH] dm-lc.c: fix for a potential NULL pointer dereference

[PATCH] dm-lc.c: fix for a potential NULL pointer dereference

[Kumar Amit Mehta]

Memory allocation may fail, hence add a check before dereferencing
the pointer.

Signed-off-by: Kumar Amit Mehta <gmate.amit@gmail.com>
---
 Driver/dm-lc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Driver/dm-lc.c b/Driver/dm-lc.c
index 4a65042..e00fb27 100644
--- a/Driver/dm-lc.c
+++ b/Driver/dm-lc.c
@@ -2671,6 +2671,8 @@ static int lc_mgr_message(struct dm_target *ti, unsigned int argc, char **argv)
 	 */
 	if (!strcasecmp(cmd, "resume_cache")) {
 		struct lc_cache *cache = kzalloc(sizeof(*cache), GFP_KERNEL);
+		if (!cache)
+			return -ENOMEM;
 
 		struct dm_dev *dev;
 		if (dm_get_device(ti, argv[1], dm_table_get_mode(ti->table),
-- 
1.8.3.1

Re: [PATCH] dm-lc.c: fix for a potential NULL pointer dereference

[Akira Hayakawa]

Thanks, Kumar.
Your patch is applied.

resume_cache,
a routine to build in-memory data structures
by reading metadata on cache device,
is so complicated in the code and the logic
to thoroughly implement the error checks.

I am wondering how I should face this problem.
Only caring about lines
that allocates large-sized memories
and forget about anything else
is what I am thinking now.
But it is clear that
it is not a way kernel module should be.

Do you guys have some thoughts on this problem?

On 7/31/13 9:09 PM, Kumar Amit Mehta wrote:
> Memory allocation may fail, hence add a check before dereferencing
> the pointer.
> 
> Signed-off-by: Kumar Amit Mehta <gmate.amit@gmail.com>
> ---
>  Driver/dm-lc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Driver/dm-lc.c b/Driver/dm-lc.c
> index 4a65042..e00fb27 100644
> --- a/Driver/dm-lc.c
> +++ b/Driver/dm-lc.c
> @@ -2671,6 +2671,8 @@ static int lc_mgr_message(struct dm_target *ti, unsigned int argc, char **argv)
>  	 */
>  	if (!strcasecmp(cmd, "resume_cache")) {
>  		struct lc_cache *cache = kzalloc(sizeof(*cache), GFP_KERNEL);
> +		if (!cache)
> +			return -ENOMEM;
>  
>  		struct dm_dev *dev;
>  		if (dm_get_device(ti, argv[1], dm_table_get_mode(ti->table),
> 

dm-lc code/design [was: Re: dm-lc.c: fix for a potential NULL pointer dereference]

[Mike Snitzer]

On Wed, Jul 31 2013 at  9:04am -0400,
Akira Hayakawa <ruby.wktk@gmail.com> wrote:

> Thanks, Kumar.
> Your patch is applied.
> 
> resume_cache,
> a routine to build in-memory data structures
> by reading metadata on cache device,
> is so complicated in the code and the logic
> to thoroughly implement the error checks.
> 
> I am wondering how I should face this problem.
> Only caring about lines
> that allocates large-sized memories
> and forget about anything else
> is what I am thinking now.
> But it is clear that
> it is not a way kernel module should be.
> 
> Do you guys have some thoughts on this problem?

I had a quick look at "resume_cache".  I was thinking you were referring
to the .resume method of the target.  The .resume method must not
allocate _any_ memory (DM convention requires all memory allocations to
be done in terms of preallocated memory or more commonly as part of the
DM table load, via .ctr)... anyway ignoring that for now.

I was very surprised to see that you're managing devices in terms of DM
messages like "resume_cache" (which I assume your dm-lc userspace tools
initiate).  This seems wrong -- I'm also not seeing proper locking in
the code either (which you get for free if with DM if you use the
traditional DM hooks via target_type ops).  But I haven't been able to
do a proper review.

DM device creation and deletion are done in terms of load (.ctr) and
remove (.dtr).

And all these sysfs files are excessive too.  You'll notice that DM devices
don't expose discrete sysfs files on a per device basis.  All per-device
info is exposed via .status

We _could_ take steps to establish a per-DM-device sysfs namespace; but
that would need to be something wired up to the DM core.  So I'd prefer
dm-lc use traditional .status (info and table).

All said, I'll try to make time for a more formal review in the coming
weeks.  Please feel free to ask more questions in the mean time.

Thanks,
Mike

Re: dm-lc code/design [was: Re: dm-lc.c: fix for a potential NULL pointer dereference]

[Akira Hayakawa]

Thanks for your comment, Mike.

> DM device creation and deletion are done in terms of load (.ctr) and
> remove (.dtr).

In dm-lc, there are two actors.

One is lc_device which is truly a DM device.
It is first created as a linear-like device
simply backed by a backing store
and later attached to a lc_cache for caching to get started.
I/Os from the upper layer like filesystems are submitted to lc_device.

The another is lc_cache which is NOT a DM device.
It is just the context of a cache device.
resume_cache routine called by lc-resume-cache command written in Python
reads metadata regions on a cache device medium
and build an in-memory structure. That is lc_cache.

I am sorry to puzzle you.
I will make a slide to explain
how these structures are built and related.
I already made a slide to explain
how writes to lc_device are processed but
I don't think that is enough for people
who want to know how dm-lc is initialized either.

> And all these sysfs files are excessive too.  You'll notice that DM
devices
> don't expose discrete sysfs files on a per device basis.  All per-device
> info is exposed via .status

dm-lc gives ID numbers to both lc_device and lc_cache
and then manages sysfs under /sys/module/dm_lc like

root@Hercules:/sys/module/dm_lc# tree devices/ caches/
devices/
└── 5
    ├── cache_id
    ├── dev
    ├── device -> ../../../../devices/virtual/block/dm-0
    ├── migrate_threshold
    └── nr_dirty_caches
caches/
└── 3
    ├── allow_migrate
    ├── barrier_deadline_ms
    ├── commit_super_block
    ├── commit_super_block_interval
    ├── device -> ../../../../devices/virtual/block/dm-1
    ├── flush_current_buffer
    ├── flush_current_buffer_interval
    ├── force_migrate
    ├── last_flushed_segment_id
    ├── last_migrated_segment_id
    ├── nr_max_batched_migration
    └── update_interval

In the case above, lc_device with ID 5 and lc_cache with ID 3
are built on memory and the lc_device uses the lc_cache.

root@Hercules:/sys/module/dm_lc# cat devices/5/cache_id
3

I know that device-mapper can not establish a sysfs
for a DM device and that's why I elaborated this workaround.
All the sysfs are placed in a subtree looks manageable.

.status is used.
The commands below can show the status,
such as static information like memory consumption
and cache statistics of the cache device.
In my architecture,
sysfs is used for variables needed to control the module behavior and
status is used for otherwise.

root@Hercules:/sys/module/dm_lc# dmsetup message lc-mgr 0 switch_to 3
root@Hercules:/sys/module/dm_lc# dmsetup status lc-mgr 0
lc-mgr: 0 1 lc-mgr
current cache_id_ptr: 3
static RAM(approx.): 37056 (byte)
allow_migrate: 1
nr_segments: 3
last_migrated_segment_id: 403
last_flushed_segment_id: 403
current segment id: 404
cursor: 255

write? hit? on_buffer? fullsize?
0 0 0 0 0
1 0 0 0 0
0 1 0 0 0
1 1 0 0 0
0 0 1 0 0
1 0 1 0 0
0 1 1 0 0
1 1 1 0 0
0 0 0 1 83
1 0 0 1 0
0 1 0 1 0
1 1 0 1 0
0 0 1 1 0
1 0 1 1 0
0 1 1 1 0

> All said, I'll try to make time for a more formal review in the coming
> weeks.  Please feel free to ask more questions in the mean time.
Thanks,
I will do my best to help your code review.
I will make the said slide on this weekend and upload to my repo.
I am really looking forward to go through the code review.

Akira

On 8/1/13 12:57 AM, Mike Snitzer wrote:
> On Wed, Jul 31 2013 at  9:04am -0400,
> Akira Hayakawa <ruby.wktk@gmail.com> wrote:
> 
>> Thanks, Kumar.
>> Your patch is applied.
>>
>> resume_cache,
>> a routine to build in-memory data structures
>> by reading metadata on cache device,
>> is so complicated in the code and the logic
>> to thoroughly implement the error checks.
>>
>> I am wondering how I should face this problem.
>> Only caring about lines
>> that allocates large-sized memories
>> and forget about anything else
>> is what I am thinking now.
>> But it is clear that
>> it is not a way kernel module should be.
>>
>> Do you guys have some thoughts on this problem?
> 
> I had a quick look at "resume_cache".  I was thinking you were referring
> to the .resume method of the target.  The .resume method must not
> allocate _any_ memory (DM convention requires all memory allocations to
> be done in terms of preallocated memory or more commonly as part of the
> DM table load, via .ctr)... anyway ignoring that for now.
> 
> I was very surprised to see that you're managing devices in terms of DM
> messages like "resume_cache" (which I assume your dm-lc userspace tools
> initiate).  This seems wrong -- I'm also not seeing proper locking in
> the code either (which you get for free if with DM if you use the
> traditional DM hooks via target_type ops).  But I haven't been able to
> do a proper review.
> 
> DM device creation and deletion are done in terms of load (.ctr) and
> remove (.dtr).
> 
> And all these sysfs files are excessive too.  You'll notice that DM devices
> don't expose discrete sysfs files on a per device basis.  All per-device
> info is exposed via .status
> 
> We _could_ take steps to establish a per-DM-device sysfs namespace; but
> that would need to be something wired up to the DM core.  So I'd prefer
> dm-lc use traditional .status (info and table).
> 
> All said, I'll try to make time for a more formal review in the coming
> weeks.  Please feel free to ask more questions in the mean time.
> 
> Thanks,
> Mike
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel