| Message ID | 20200819141745.11005-1-sergeygo@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | srp_daemon: Avoid extra permissions for the lock file | expand |
> From: Sergey Gorenko <sergeygo@nvidia.com> > Sent: Wednesday, August 19, 2020 5:18 PM > To: bvanassche@acm.org > Cc: linux-rdma@vger.kernel.org; Sergey Gorenko <sergeygo@nvidia.com>; > Max Gurtovoy <mgurtovoy@nvidia.com> > Subject: [PATCH] srp_daemon: Avoid extra permissions for the lock file > > There is no need to create a world-writable lock file. > It's enough to have an RW permission for the file owner only. > > Signed-off-by: Sergey Gorenko <sergeygo@nvidia.com> > Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com> > --- > srp_daemon/srp_daemon.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c index > f14d9f56c9f2..fcf94537cebb 100644 > --- a/srp_daemon/srp_daemon.c > +++ b/srp_daemon/srp_daemon.c > @@ -142,7 +142,6 @@ static int check_process_uniqueness(struct config_t > *conf) > return -1; > } > > - fchmod(fd, > S_IRUSR|S_IRGRP|S_IROTH|S_IWUSR|S_IWGRP|S_IWOTH); > if (0 != lockf(fd, F_TLOCK, 0)) { > pr_err("failed to lock %s (errno: %d). possibly another " > "srp_daemon is locking it\n", path, errno); > -- > 2.21.1 Hi Bart, Could you review the patch? I'm asking for you because you are specified as a maintainer for srp_daemon in rdma-core. Regards, Sergey
On 2020-09-14 02:24, Sergey Gorenko wrote: > Could you review the patch? I'm asking for you because you are specified > as a maintainer for srp_daemon in rdma-core. Thanks for the reminder. That patch had escaped from my attention. Bart.
On 2020-08-19 07:17, Sergey Gorenko wrote: > There is no need to create a world-writable lock file. > It's enough to have an RW permission for the file owner only. > > Signed-off-by: Sergey Gorenko <sergeygo@nvidia.com> > Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com> > --- > srp_daemon/srp_daemon.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c > index f14d9f56c9f2..fcf94537cebb 100644 > --- a/srp_daemon/srp_daemon.c > +++ b/srp_daemon/srp_daemon.c > @@ -142,7 +142,6 @@ static int check_process_uniqueness(struct config_t *conf) > return -1; > } > > - fchmod(fd, S_IRUSR|S_IRGRP|S_IROTH|S_IWUSR|S_IWGRP|S_IWOTH); > if (0 != lockf(fd, F_TLOCK, 0)) { > pr_err("failed to lock %s (errno: %d). possibly another " > "srp_daemon is locking it\n", path, errno); I think the fchmod() call was introduced by commit ee138ce1e40d ("Cause srp_daemon launch to fail if another srp_daemon is already working on the same HCA port."). Has it been verified that with this change applied that mechanism still works? Anyway, please add a reference to that commit in the patch description. Thanks, Bart.
> -----Original Message----- > From: Bart Van Assche <bvanassche@acm.org> > Sent: Monday, September 14, 2020 7:56 PM > To: Sergey Gorenko <sergeygo@nvidia.com> > Cc: linux-rdma@vger.kernel.org; Max Gurtovoy <mgurtovoy@nvidia.com> > Subject: Re: [PATCH] srp_daemon: Avoid extra permissions for the lock file > > On 2020-08-19 07:17, Sergey Gorenko wrote: > > There is no need to create a world-writable lock file. > > It's enough to have an RW permission for the file owner only. > > > > Signed-off-by: Sergey Gorenko <sergeygo@nvidia.com> > > Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com> > > --- > > srp_daemon/srp_daemon.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c index > > f14d9f56c9f2..fcf94537cebb 100644 > > --- a/srp_daemon/srp_daemon.c > > +++ b/srp_daemon/srp_daemon.c > > @@ -142,7 +142,6 @@ static int check_process_uniqueness(struct > config_t *conf) > > return -1; > > } > > > > - fchmod(fd, > S_IRUSR|S_IRGRP|S_IROTH|S_IWUSR|S_IWGRP|S_IWOTH); > > if (0 != lockf(fd, F_TLOCK, 0)) { > > pr_err("failed to lock %s (errno: %d). possibly another " > > "srp_daemon is locking it\n", path, errno); > > I think the fchmod() call was introduced by commit ee138ce1e40d ("Cause > srp_daemon launch to fail if another srp_daemon is already working on the > same HCA port."). Has it been verified that with this change applied that > mechanism still works? > > Anyway, please add a reference to that commit in the patch description. > > Thanks, > > Bart. > Bart, I tested the patch for the following scenarios: * Start the srp_daemon service when srp_daemon is not running and the lock file does not exist. * Start the srp_daemon service when srp_daemon is not running and the lock file exists. * Start the srp_daemon service when srp_daemon is running and the lock file exists. * Start the srp_daemon service when srp_daemon is running and the lock file exists and the file owner is not root. (Such scenario can happen if someone tries to run srp_daemon manually as not root. The srp_daemon fails in this case, but the lock file is created). This case is handled successfully even without the fchmod() call because the srp_daemon service starts srp_daemon as root. I do not know any case when fchmod() is needed. And it does not look like a good idea to create a word-writable file owned by root. That's why I want to remove the fchmod() call. Do you have an idea when the fchmod() call can be needed? If you have no other objections, I will add the fixes line and send V1. Thanks, Sergey
On 2020-09-15 01:10, Sergey Gorenko wrote: > I tested the patch for the following scenarios: > * Start the srp_daemon service when srp_daemon is not running and the lock file does not exist. > * Start the srp_daemon service when srp_daemon is not running and the lock file exists. > * Start the srp_daemon service when srp_daemon is running and the lock file exists. > * Start the srp_daemon service when srp_daemon is running and the lock file exists and the file owner is not root. (Such scenario can happen if someone tries to run srp_daemon manually as not root. The srp_daemon fails in this case, but the lock file is created). This case is handled successfully even without the fchmod() call because the srp_daemon service starts srp_daemon as root. > > I do not know any case when fchmod() is needed. And it does not look like a good idea to create a word-writable file owned by root. That's why I want to remove the fchmod() call. > > Do you have an idea when the fchmod() call can be needed? > > If you have no other objections, I will add the fixes line and send V1. Thanks Sergey for having shared all this information. I think this testing is sufficient. Hence: Reviewed-by: Bart Van Assche <bvanassche@acm.org>
diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c index f14d9f56c9f2..fcf94537cebb 100644 --- a/srp_daemon/srp_daemon.c +++ b/srp_daemon/srp_daemon.c @@ -142,7 +142,6 @@ static int check_process_uniqueness(struct config_t *conf) return -1; } - fchmod(fd, S_IRUSR|S_IRGRP|S_IROTH|S_IWUSR|S_IWGRP|S_IWOTH); if (0 != lockf(fd, F_TLOCK, 0)) { pr_err("failed to lock %s (errno: %d). possibly another " "srp_daemon is locking it\n", path, errno);