diff options
author | Konsta Holtta <kholtta@nvidia.com> | 2017-08-31 06:01:26 -0400 |
---|---|---|
committer | mobile promotions <svcmobile_promotions@nvidia.com> | 2017-09-11 13:34:57 -0400 |
commit | 17451138cf60f5d64eed88cc5defd44981926d9d (patch) | |
tree | ea335a05d9e038876d42bcd8a73c56e3b60af1a5 /drivers/gpu/nvgpu | |
parent | de0ce3e017decadd3a5049477f17ba770ddf074c (diff) |
gpu: nvgpu: hold ch ref when getting ch from fd
Fix a race condition in gk20a_get_channel_from_file() that returns a
channel pointer from an fd: take a reference to the channel before
putting the file ref back. Now the caller is responsible of releasing
the channel reference eventually.
Also document why dbg_session_channel_data has to hold a ref to the
channel file instead of just the channel: that might deadlock if the fds
were closed in "wrong" order.
Change-Id: I8e91b809f5f7b1cb0c1487bd955ad6d643727a53
Signed-off-by: Konsta Holtta <kholtta@nvidia.com>
Reviewed-on: https://git-master.nvidia.com/r/1549290
Reviewed-by: mobile promotions <svcmobile_promotions@nvidia.com>
Tested-by: mobile promotions <svcmobile_promotions@nvidia.com>
Diffstat (limited to 'drivers/gpu/nvgpu')
-rw-r--r-- | drivers/gpu/nvgpu/common/linux/ioctl_as.c | 11 | ||||
-rw-r--r-- | drivers/gpu/nvgpu/common/linux/ioctl_channel.c | 10 | ||||
-rw-r--r-- | drivers/gpu/nvgpu/common/linux/ioctl_ctrl.c | 3 | ||||
-rw-r--r-- | drivers/gpu/nvgpu/common/linux/ioctl_tsg.c | 2 | ||||
-rw-r--r-- | drivers/gpu/nvgpu/gk20a/dbg_gpu_gk20a.c | 39 | ||||
-rw-r--r-- | drivers/gpu/nvgpu/gk20a/dbg_gpu_gk20a.h | 9 |
6 files changed, 58 insertions, 16 deletions
diff --git a/drivers/gpu/nvgpu/common/linux/ioctl_as.c b/drivers/gpu/nvgpu/common/linux/ioctl_as.c index 6fd0a3d2..d4242955 100644 --- a/drivers/gpu/nvgpu/common/linux/ioctl_as.c +++ b/drivers/gpu/nvgpu/common/linux/ioctl_as.c | |||
@@ -42,14 +42,19 @@ static int gk20a_as_ioctl_bind_channel( | |||
42 | gk20a_dbg_fn(""); | 42 | gk20a_dbg_fn(""); |
43 | 43 | ||
44 | ch = gk20a_get_channel_from_file(args->channel_fd); | 44 | ch = gk20a_get_channel_from_file(args->channel_fd); |
45 | if (!ch || gk20a_channel_as_bound(ch)) | 45 | if (!ch) |
46 | return -EINVAL; | 46 | return -EINVAL; |
47 | 47 | ||
48 | if (gk20a_channel_as_bound(ch)) { | ||
49 | err = -EINVAL; | ||
50 | goto out; | ||
51 | } | ||
52 | |||
48 | /* this will set channel_gk20a->vm */ | 53 | /* this will set channel_gk20a->vm */ |
49 | err = ch->g->ops.mm.vm_bind_channel(as_share, ch); | 54 | err = ch->g->ops.mm.vm_bind_channel(as_share, ch); |
50 | if (err) | ||
51 | return err; | ||
52 | 55 | ||
56 | out: | ||
57 | gk20a_channel_put(ch); | ||
53 | return err; | 58 | return err; |
54 | } | 59 | } |
55 | 60 | ||
diff --git a/drivers/gpu/nvgpu/common/linux/ioctl_channel.c b/drivers/gpu/nvgpu/common/linux/ioctl_channel.c index ab02dc42..5e09c677 100644 --- a/drivers/gpu/nvgpu/common/linux/ioctl_channel.c +++ b/drivers/gpu/nvgpu/common/linux/ioctl_channel.c | |||
@@ -260,8 +260,15 @@ static int gk20a_init_error_notifier(struct channel_gk20a *ch, | |||
260 | return 0; | 260 | return 0; |
261 | } | 261 | } |
262 | 262 | ||
263 | /* | ||
264 | * This returns the channel with a reference. The caller must | ||
265 | * gk20a_channel_put() the ref back after use. | ||
266 | * | ||
267 | * NULL is returned if the channel was not found. | ||
268 | */ | ||
263 | struct channel_gk20a *gk20a_get_channel_from_file(int fd) | 269 | struct channel_gk20a *gk20a_get_channel_from_file(int fd) |
264 | { | 270 | { |
271 | struct channel_gk20a *ch; | ||
265 | struct channel_priv *priv; | 272 | struct channel_priv *priv; |
266 | struct file *f = fget(fd); | 273 | struct file *f = fget(fd); |
267 | 274 | ||
@@ -274,8 +281,9 @@ struct channel_gk20a *gk20a_get_channel_from_file(int fd) | |||
274 | } | 281 | } |
275 | 282 | ||
276 | priv = (struct channel_priv *)f->private_data; | 283 | priv = (struct channel_priv *)f->private_data; |
284 | ch = gk20a_channel_get(priv->c); | ||
277 | fput(f); | 285 | fput(f); |
278 | return priv->c; | 286 | return ch; |
279 | } | 287 | } |
280 | 288 | ||
281 | int gk20a_channel_release(struct inode *inode, struct file *filp) | 289 | int gk20a_channel_release(struct inode *inode, struct file *filp) |
diff --git a/drivers/gpu/nvgpu/common/linux/ioctl_ctrl.c b/drivers/gpu/nvgpu/common/linux/ioctl_ctrl.c index 2c274f23..0d79b143 100644 --- a/drivers/gpu/nvgpu/common/linux/ioctl_ctrl.c +++ b/drivers/gpu/nvgpu/common/linux/ioctl_ctrl.c | |||
@@ -354,6 +354,8 @@ static int nvgpu_gpu_ioctl_inval_icache( | |||
354 | nvgpu_mutex_acquire(&g->dbg_sessions_lock); | 354 | nvgpu_mutex_acquire(&g->dbg_sessions_lock); |
355 | err = g->ops.gr.inval_icache(g, ch); | 355 | err = g->ops.gr.inval_icache(g, ch); |
356 | nvgpu_mutex_release(&g->dbg_sessions_lock); | 356 | nvgpu_mutex_release(&g->dbg_sessions_lock); |
357 | |||
358 | gk20a_channel_put(ch); | ||
357 | return err; | 359 | return err; |
358 | } | 360 | } |
359 | 361 | ||
@@ -393,6 +395,7 @@ static int nvgpu_gpu_ioctl_set_debug_mode( | |||
393 | err = -ENOSYS; | 395 | err = -ENOSYS; |
394 | nvgpu_mutex_release(&g->dbg_sessions_lock); | 396 | nvgpu_mutex_release(&g->dbg_sessions_lock); |
395 | 397 | ||
398 | gk20a_channel_put(ch); | ||
396 | return err; | 399 | return err; |
397 | } | 400 | } |
398 | 401 | ||
diff --git a/drivers/gpu/nvgpu/common/linux/ioctl_tsg.c b/drivers/gpu/nvgpu/common/linux/ioctl_tsg.c index c68c907e..d35ea14c 100644 --- a/drivers/gpu/nvgpu/common/linux/ioctl_tsg.c +++ b/drivers/gpu/nvgpu/common/linux/ioctl_tsg.c | |||
@@ -49,6 +49,8 @@ static int gk20a_tsg_bind_channel_fd(struct tsg_gk20a *tsg, int ch_fd) | |||
49 | return -EINVAL; | 49 | return -EINVAL; |
50 | 50 | ||
51 | err = ch->g->ops.fifo.tsg_bind_channel(tsg, ch); | 51 | err = ch->g->ops.fifo.tsg_bind_channel(tsg, ch); |
52 | |||
53 | gk20a_channel_put(ch); | ||
52 | return err; | 54 | return err; |
53 | } | 55 | } |
54 | 56 | ||
diff --git a/drivers/gpu/nvgpu/gk20a/dbg_gpu_gk20a.c b/drivers/gpu/nvgpu/gk20a/dbg_gpu_gk20a.c index 00050850..19433df9 100644 --- a/drivers/gpu/nvgpu/gk20a/dbg_gpu_gk20a.c +++ b/drivers/gpu/nvgpu/gk20a/dbg_gpu_gk20a.c | |||
@@ -514,7 +514,8 @@ static int dbg_unbind_channel_gk20a(struct dbg_session_gk20a *dbg_s, | |||
514 | 514 | ||
515 | if (!channel_found) { | 515 | if (!channel_found) { |
516 | gk20a_dbg_fn("channel not bounded, fd=%d\n", args->channel_fd); | 516 | gk20a_dbg_fn("channel not bounded, fd=%d\n", args->channel_fd); |
517 | return -EINVAL; | 517 | err = -EINVAL; |
518 | goto out; | ||
518 | } | 519 | } |
519 | 520 | ||
520 | nvgpu_mutex_acquire(&g->dbg_sessions_lock); | 521 | nvgpu_mutex_acquire(&g->dbg_sessions_lock); |
@@ -523,6 +524,8 @@ static int dbg_unbind_channel_gk20a(struct dbg_session_gk20a *dbg_s, | |||
523 | nvgpu_mutex_release(&dbg_s->ch_list_lock); | 524 | nvgpu_mutex_release(&dbg_s->ch_list_lock); |
524 | nvgpu_mutex_release(&g->dbg_sessions_lock); | 525 | nvgpu_mutex_release(&g->dbg_sessions_lock); |
525 | 526 | ||
527 | out: | ||
528 | gk20a_channel_put(ch); | ||
526 | return err; | 529 | return err; |
527 | } | 530 | } |
528 | 531 | ||
@@ -584,13 +587,16 @@ static int dbg_bind_channel_gk20a(struct dbg_session_gk20a *dbg_s, | |||
584 | struct channel_gk20a *ch; | 587 | struct channel_gk20a *ch; |
585 | struct dbg_session_channel_data *ch_data; | 588 | struct dbg_session_channel_data *ch_data; |
586 | struct dbg_session_data *session_data; | 589 | struct dbg_session_data *session_data; |
590 | int err = 0; | ||
587 | 591 | ||
588 | gk20a_dbg(gpu_dbg_fn|gpu_dbg_gpu_dbg, "%s fd=%d", | 592 | gk20a_dbg(gpu_dbg_fn|gpu_dbg_gpu_dbg, "%s fd=%d", |
589 | g->name, args->channel_fd); | 593 | g->name, args->channel_fd); |
590 | 594 | ||
591 | /* even though get_file_channel is doing this it releases it as well */ | 595 | /* |
592 | /* by holding it here we'll keep it from disappearing while the | 596 | * Although gk20a_get_channel_from_file gives us a channel ref, need to |
593 | * debugger is in session */ | 597 | * hold a ref to the file during the session lifetime. See comment in |
598 | * struct dbg_session_channel_data. | ||
599 | */ | ||
594 | f = fget(args->channel_fd); | 600 | f = fget(args->channel_fd); |
595 | if (!f) | 601 | if (!f) |
596 | return -ENODEV; | 602 | return -ENODEV; |
@@ -598,8 +604,8 @@ static int dbg_bind_channel_gk20a(struct dbg_session_gk20a *dbg_s, | |||
598 | ch = gk20a_get_channel_from_file(args->channel_fd); | 604 | ch = gk20a_get_channel_from_file(args->channel_fd); |
599 | if (!ch) { | 605 | if (!ch) { |
600 | gk20a_dbg_fn("no channel found for fd"); | 606 | gk20a_dbg_fn("no channel found for fd"); |
601 | fput(f); | 607 | err = -EINVAL; |
602 | return -EINVAL; | 608 | goto out_fput; |
603 | } | 609 | } |
604 | 610 | ||
605 | gk20a_dbg_fn("%s hwchid=%d", g->name, ch->chid); | 611 | gk20a_dbg_fn("%s hwchid=%d", g->name, ch->chid); |
@@ -609,8 +615,8 @@ static int dbg_bind_channel_gk20a(struct dbg_session_gk20a *dbg_s, | |||
609 | 615 | ||
610 | ch_data = nvgpu_kzalloc(g, sizeof(*ch_data)); | 616 | ch_data = nvgpu_kzalloc(g, sizeof(*ch_data)); |
611 | if (!ch_data) { | 617 | if (!ch_data) { |
612 | fput(f); | 618 | err = -ENOMEM; |
613 | return -ENOMEM; | 619 | goto out_chput; |
614 | } | 620 | } |
615 | ch_data->ch_f = f; | 621 | ch_data->ch_f = f; |
616 | ch_data->channel_fd = args->channel_fd; | 622 | ch_data->channel_fd = args->channel_fd; |
@@ -619,9 +625,8 @@ static int dbg_bind_channel_gk20a(struct dbg_session_gk20a *dbg_s, | |||
619 | 625 | ||
620 | session_data = nvgpu_kzalloc(g, sizeof(*session_data)); | 626 | session_data = nvgpu_kzalloc(g, sizeof(*session_data)); |
621 | if (!session_data) { | 627 | if (!session_data) { |
622 | nvgpu_kfree(g, ch_data); | 628 | err = -ENOMEM; |
623 | fput(f); | 629 | goto out_kfree; |
624 | return -ENOMEM; | ||
625 | } | 630 | } |
626 | session_data->dbg_s = dbg_s; | 631 | session_data->dbg_s = dbg_s; |
627 | nvgpu_init_list_node(&session_data->dbg_s_entry); | 632 | nvgpu_init_list_node(&session_data->dbg_s_entry); |
@@ -636,7 +641,19 @@ static int dbg_bind_channel_gk20a(struct dbg_session_gk20a *dbg_s, | |||
636 | nvgpu_mutex_release(&ch->dbg_s_lock); | 641 | nvgpu_mutex_release(&ch->dbg_s_lock); |
637 | nvgpu_mutex_release(&g->dbg_sessions_lock); | 642 | nvgpu_mutex_release(&g->dbg_sessions_lock); |
638 | 643 | ||
644 | gk20a_channel_put(ch); | ||
645 | |||
639 | return 0; | 646 | return 0; |
647 | |||
648 | out_kfree: | ||
649 | nvgpu_kfree(g, ch_data); | ||
650 | out_chput: | ||
651 | gk20a_channel_put(ch); | ||
652 | nvgpu_mutex_release(&ch->dbg_s_lock); | ||
653 | nvgpu_mutex_release(&g->dbg_sessions_lock); | ||
654 | out_fput: | ||
655 | fput(f); | ||
656 | return err; | ||
640 | } | 657 | } |
641 | 658 | ||
642 | static int nvgpu_ioctl_channel_reg_ops(struct dbg_session_gk20a *dbg_s, | 659 | static int nvgpu_ioctl_channel_reg_ops(struct dbg_session_gk20a *dbg_s, |
diff --git a/drivers/gpu/nvgpu/gk20a/dbg_gpu_gk20a.h b/drivers/gpu/nvgpu/gk20a/dbg_gpu_gk20a.h index 827fb42f..4d2c0f94 100644 --- a/drivers/gpu/nvgpu/gk20a/dbg_gpu_gk20a.h +++ b/drivers/gpu/nvgpu/gk20a/dbg_gpu_gk20a.h | |||
@@ -93,7 +93,14 @@ dbg_session_data_from_dbg_s_entry(struct nvgpu_list_node *node) | |||
93 | }; | 93 | }; |
94 | 94 | ||
95 | struct dbg_session_channel_data { | 95 | struct dbg_session_channel_data { |
96 | struct file *ch_f; | 96 | /* |
97 | * We have to keep a ref to the _file_, not the channel, because | ||
98 | * close(channel_fd) is synchronous and would deadlock if we had an | ||
99 | * open debug session fd holding a channel ref at that time. Holding a | ||
100 | * ref to the file makes close(channel_fd) just drop a kernel ref to | ||
101 | * the file; the channel will close when the last file ref is dropped. | ||
102 | */ | ||
103 | struct file *ch_f; | ||
97 | int channel_fd; | 104 | int channel_fd; |
98 | int chid; | 105 | int chid; |
99 | struct nvgpu_list_node ch_entry; | 106 | struct nvgpu_list_node ch_entry; |