* [REVIEW][PATCH 12/15] userns: Convert drm to use kuid and kgid and struct pid where appropriate
From: Eric W. Biederman @ 2012-08-26 0:05 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, linux-fsdevel, Serge E. Hallyn, David Miller,
David Airlie, dri-devel
In-Reply-To: <87lih2h6i4.fsf@xmission.com>
Blink Blink this had not been converted to use struct pid ages ago?
- On drm open capture the openers kuid and struct pid.
- On drm close release the kuid and struct pid
- When reporting the uid and pid convert the kuid and struct pid
into values in the appropriate namespace.
Cc: David Airlie <airlied@linux.ie>
Cc: dri-devel@lists.freedesktop.org
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/gpu/drm/drm_fops.c | 3 ++-
drivers/gpu/drm/drm_info.c | 5 +++--
drivers/gpu/drm/drm_ioctl.c | 4 ++--
include/drm/drmP.h | 4 ++--
init/Kconfig | 1 -
5 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 5062eec..433d2fa 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -251,7 +251,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
filp->private_data = priv;
priv->filp = filp;
priv->uid = current_euid();
- priv->pid = task_pid_nr(current);
+ priv->pid = get_pid(task_pid(current));
priv->minor = idr_find(&drm_minors_idr, minor_id);
priv->ioctl_count = 0;
/* for compatibility root is always authenticated */
@@ -524,6 +524,7 @@ int drm_release(struct inode *inode, struct file *filp)
if (drm_core_check_feature(dev, DRIVER_PRIME))
drm_prime_destroy_file_private(&file_priv->prime);
+ put_pid(file_priv->pid);
kfree(file_priv);
/* ========================================================
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
index 8928edb..eb0af39 100644
--- a/drivers/gpu/drm/drm_info.c
+++ b/drivers/gpu/drm/drm_info.c
@@ -191,8 +191,9 @@ int drm_clients_info(struct seq_file *m, void *data)
seq_printf(m, "%c %3d %5d %5d %10u %10lu\n",
priv->authenticated ? 'y' : 'n',
priv->minor->index,
- priv->pid,
- priv->uid, priv->magic, priv->ioctl_count);
+ pid_vnr(priv->pid),
+ from_kuid_munged(seq_user_ns(m), priv->uid),
+ priv->magic, priv->ioctl_count);
}
mutex_unlock(&dev->struct_mutex);
return 0;
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 64a62c6..39a4383 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -215,8 +215,8 @@ int drm_getclient(struct drm_device *dev, void *data,
list_for_each_entry(pt, &dev->filelist, lhead) {
if (i++ >= idx) {
client->auth = pt->authenticated;
- client->pid = pt->pid;
- client->uid = pt->uid;
+ client->pid = pid_vnr(pt->pid);
+ client->uid = from_kuid_munged(current_user_ns(), pt->uid);
client->magic = pt->magic;
client->iocs = pt->ioctl_count;
mutex_unlock(&dev->struct_mutex);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index d6b67bb..9bc5c6a 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -426,8 +426,8 @@ struct drm_prime_file_private {
/** File private data */
struct drm_file {
int authenticated;
- pid_t pid;
- uid_t uid;
+ struct pid *pid;
+ kuid_t uid;
drm_magic_t magic;
unsigned long ioctl_count;
struct list_head lhead;
diff --git a/init/Kconfig b/init/Kconfig
index d849ba2..2a388e5 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -930,7 +930,6 @@ config UIDGID_CONVERTED
depends on FS_POSIX_ACL = n
depends on QUOTA = n
depends on QUOTACTL = n
- depends on DRM = n
# Networking
depends on NET_9P = n
--
1.7.5.4
^ permalink raw reply related
* [REVIEW][PATCH 11/15] userns: Teach trace to use from_kuid
From: Eric W. Biederman @ 2012-08-26 0:04 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, linux-fsdevel, Serge E. Hallyn, David Miller,
Steven Rostedt, Frederic Weisbecker, Ingo Molnar
In-Reply-To: <87lih2h6i4.fsf@xmission.com>
- When tracing capture the kuid.
- When displaying the data to user space convert the kuid into the
user namespace of the process that opened the report file.
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
init/Kconfig | 1 -
kernel/trace/trace.c | 3 ++-
kernel/trace/trace.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/init/Kconfig b/init/Kconfig
index af41fa7..d849ba2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -927,7 +927,6 @@ config UIDGID_CONVERTED
# Features
depends on IMA = n
depends on EVM = n
- depends on TRACING = n
depends on FS_POSIX_ACL = n
depends on QUOTA = n
depends on QUOTACTL = n
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 5c38c81..c9ace83 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2060,7 +2060,8 @@ print_trace_header(struct seq_file *m, struct trace_iterator *iter)
seq_puts(m, "# -----------------\n");
seq_printf(m, "# | task: %.16s-%d "
"(uid:%d nice:%ld policy:%ld rt_prio:%ld)\n",
- data->comm, data->pid, data->uid, data->nice,
+ data->comm, data->pid,
+ from_kuid_munged(seq_user_ns(m), data->uid), data->nice,
data->policy, data->rt_priority);
seq_puts(m, "# -----------------\n");
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 55e1f7f..40a6f30 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -147,7 +147,7 @@ struct trace_array_cpu {
unsigned long skipped_entries;
cycle_t preempt_timestamp;
pid_t pid;
- uid_t uid;
+ kuid_t uid;
char comm[TASK_COMM_LEN];
};
--
1.7.5.4
^ permalink raw reply related
* [REVIEW][PATCH 10/15] userns: Convert debugfs to use kuid/kgid where appropriate.
From: Eric W. Biederman @ 2012-08-26 0:03 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, linux-fsdevel, Serge E. Hallyn, David Miller,
Greg Kroah-Hartman
In-Reply-To: <87lih2h6i4.fsf@xmission.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/debugfs/inode.c | 26 ++++++++++++++++++--------
init/Kconfig | 1 -
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 4733eab..36e2b66 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -128,8 +128,8 @@ static inline int debugfs_positive(struct dentry *dentry)
}
struct debugfs_mount_opts {
- uid_t uid;
- gid_t gid;
+ kuid_t uid;
+ kgid_t gid;
umode_t mode;
};
@@ -156,6 +156,8 @@ static int debugfs_parse_options(char *data, struct debugfs_mount_opts *opts)
substring_t args[MAX_OPT_ARGS];
int option;
int token;
+ kuid_t uid;
+ kgid_t gid;
char *p;
opts->mode = DEBUGFS_DEFAULT_MODE;
@@ -169,12 +171,18 @@ static int debugfs_parse_options(char *data, struct debugfs_mount_opts *opts)
case Opt_uid:
if (match_int(&args[0], &option))
return -EINVAL;
- opts->uid = option;
+ uid = make_kuid(current_user_ns(), option);
+ if (!uid_valid(uid))
+ return -EINVAL;
+ opts->uid = uid;
break;
case Opt_gid:
if (match_octal(&args[0], &option))
return -EINVAL;
- opts->gid = option;
+ gid = make_kgid(current_user_ns(), option);
+ if (!gid_valid(gid))
+ return -EINVAL;
+ opts->gid = gid;
break;
case Opt_mode:
if (match_octal(&args[0], &option))
@@ -226,10 +234,12 @@ static int debugfs_show_options(struct seq_file *m, struct dentry *root)
struct debugfs_fs_info *fsi = root->d_sb->s_fs_info;
struct debugfs_mount_opts *opts = &fsi->mount_opts;
- if (opts->uid != 0)
- seq_printf(m, ",uid=%u", opts->uid);
- if (opts->gid != 0)
- seq_printf(m, ",gid=%u", opts->gid);
+ if (!uid_eq(opts->uid, GLOBAL_ROOT_UID))
+ seq_printf(m, ",uid=%u",
+ from_kuid_munged(&init_user_ns, opts->uid));
+ if (!gid_eq(opts->gid, GLOBAL_ROOT_GID))
+ seq_printf(m, ",gid=%u",
+ from_kgid_munged(&init_user_ns, opts->gid));
if (opts->mode != DEBUGFS_DEFAULT_MODE)
seq_printf(m, ",mode=%o", opts->mode);
diff --git a/init/Kconfig b/init/Kconfig
index 7327869..af41fa7 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -955,7 +955,6 @@ config UIDGID_CONVERTED
depends on CODA_FS = n
depends on CONFIGFS_FS = n
depends on CRAMFS = n
- depends on DEBUG_FS = n
depends on ECRYPT_FS = n
depends on EFS_FS = n
depends on EXOFS_FS = n
--
1.7.5.4
^ permalink raw reply related
* [REVIEW][PATCH 09/15] userns: Convert process event connector to handle kuids and kgids
From: Eric W. Biederman @ 2012-08-26 0:02 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, linux-fsdevel, Serge E. Hallyn, David Miller,
Evgeniy Polyakov, David Miller
In-Reply-To: <87lih2h6i4.fsf@xmission.com>
- Only allow asking for events from the initial user and pid namespace,
where we generate the events in.
- Convert kuids and kgids into the initial user namespace to report
them via the process event connector.
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Cc: David Miller <davem@davemloft.net>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/connector/cn_proc.c | 18 ++++++++++++++----
init/Kconfig | 1 -
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 3e92b7d..fce2000 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -30,6 +30,7 @@
#include <linux/gfp.h>
#include <linux/ptrace.h>
#include <linux/atomic.h>
+#include <linux/pid_namespace.h>
#include <asm/unaligned.h>
@@ -127,11 +128,11 @@ void proc_id_connector(struct task_struct *task, int which_id)
rcu_read_lock();
cred = __task_cred(task);
if (which_id == PROC_EVENT_UID) {
- ev->event_data.id.r.ruid = cred->uid;
- ev->event_data.id.e.euid = cred->euid;
+ ev->event_data.id.r.ruid = from_kuid_munged(&init_user_ns, cred->uid);
+ ev->event_data.id.e.euid = from_kuid_munged(&init_user_ns, cred->euid);
} else if (which_id == PROC_EVENT_GID) {
- ev->event_data.id.r.rgid = cred->gid;
- ev->event_data.id.e.egid = cred->egid;
+ ev->event_data.id.r.rgid = from_kgid_munged(&init_user_ns, cred->gid);
+ ev->event_data.id.e.egid = from_kgid_munged(&init_user_ns, cred->egid);
} else {
rcu_read_unlock();
return;
@@ -303,6 +304,15 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
if (msg->len != sizeof(*mc_op))
return;
+ /*
+ * Events are reported with respect to the initial pid
+ * and user namespaces so ignore requestors from
+ * other namespaces.
+ */
+ if ((current_user_ns() != &init_user_ns) ||
+ (task_active_pid_ns(current) != &init_pid_ns))
+ return;
+
mc_op = (enum proc_cn_mcast_op *)msg->data;
switch (*mc_op) {
case PROC_CN_MCAST_LISTEN:
diff --git a/init/Kconfig b/init/Kconfig
index 6c9d004..7327869 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -932,7 +932,6 @@ config UIDGID_CONVERTED
depends on QUOTA = n
depends on QUOTACTL = n
depends on DRM = n
- depends on PROC_EVENTS = n
# Networking
depends on NET_9P = n
--
1.7.5.4
^ permalink raw reply related
* [REVIEW][PATCH 07/15] userns: Convert taskstats to handle the user and pid namespaces.
From: Eric W. Biederman @ 2012-08-26 0:01 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, linux-fsdevel, Serge E. Hallyn, David Miller,
Balbir Singh
In-Reply-To: <87lih2h6i4.fsf@xmission.com>
- Explicitly limit exit task stat broadcast to the initial user and
pid namespaces, as it is already limited to the initial network
namespace.
- For broadcast task stats explicitly generate all of the idenitiers
in terms of the initial user namespace and the initial pid
namespace.
- For request stats report them in terms of the current user namespace
and the current pid namespace. Netlink messages are delivered
syncrhonously to the kernel allowing us to get the user namespace
and the pid namespace from the current task.
- Pass the namespaces for representing pids and uids and gids
into bacct_add_task.
Cc: Balbir Singh <bsingharora@gmail.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
include/linux/tsacct_kern.h | 8 ++++++--
init/Kconfig | 1 -
kernel/taskstats.c | 23 +++++++++++++++++------
kernel/tsacct.c | 12 +++++++-----
4 files changed, 30 insertions(+), 14 deletions(-)
diff --git a/include/linux/tsacct_kern.h b/include/linux/tsacct_kern.h
index 7e50ac7..44893e5 100644
--- a/include/linux/tsacct_kern.h
+++ b/include/linux/tsacct_kern.h
@@ -10,9 +10,13 @@
#include <linux/taskstats.h>
#ifdef CONFIG_TASKSTATS
-extern void bacct_add_tsk(struct taskstats *stats, struct task_struct *tsk);
+extern void bacct_add_tsk(struct user_namespace *user_ns,
+ struct pid_namespace *pid_ns,
+ struct taskstats *stats, struct task_struct *tsk);
#else
-static inline void bacct_add_tsk(struct taskstats *stats, struct task_struct *tsk)
+static inline void bacct_add_tsk(struct user_namespace *user_ns,
+ struct pid_namespace *pid_ns,
+ struct taskstats *stats, struct task_struct *tsk)
{}
#endif /* CONFIG_TASKSTATS */
diff --git a/init/Kconfig b/init/Kconfig
index 525f4e8..965a700 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -927,7 +927,6 @@ config UIDGID_CONVERTED
# Features
depends on IMA = n
depends on EVM = n
- depends on TASKSTATS = n
depends on TRACING = n
depends on FS_POSIX_ACL = n
depends on QUOTA = n
diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index d0a3279..3880df2 100644
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -27,6 +27,7 @@
#include <linux/cgroup.h>
#include <linux/fs.h>
#include <linux/file.h>
+#include <linux/pid_namespace.h>
#include <net/genetlink.h>
#include <linux/atomic.h>
@@ -174,7 +175,9 @@ static void send_cpu_listeners(struct sk_buff *skb,
up_write(&listeners->sem);
}
-static void fill_stats(struct task_struct *tsk, struct taskstats *stats)
+static void fill_stats(struct user_namespace *user_ns,
+ struct pid_namespace *pid_ns,
+ struct task_struct *tsk, struct taskstats *stats)
{
memset(stats, 0, sizeof(*stats));
/*
@@ -190,7 +193,7 @@ static void fill_stats(struct task_struct *tsk, struct taskstats *stats)
stats->version = TASKSTATS_VERSION;
stats->nvcsw = tsk->nvcsw;
stats->nivcsw = tsk->nivcsw;
- bacct_add_tsk(stats, tsk);
+ bacct_add_tsk(user_ns, pid_ns, stats, tsk);
/* fill in extended acct fields */
xacct_add_tsk(stats, tsk);
@@ -207,7 +210,7 @@ static int fill_stats_for_pid(pid_t pid, struct taskstats *stats)
rcu_read_unlock();
if (!tsk)
return -ESRCH;
- fill_stats(tsk, stats);
+ fill_stats(current_user_ns(), task_active_pid_ns(current), tsk, stats);
put_task_struct(tsk);
return 0;
}
@@ -291,6 +294,12 @@ static int add_del_listener(pid_t pid, const struct cpumask *mask, int isadd)
if (!cpumask_subset(mask, cpu_possible_mask))
return -EINVAL;
+ if (current_user_ns() != &init_user_ns)
+ return -EINVAL;
+
+ if (task_active_pid_ns(current) != &init_pid_ns)
+ return -EINVAL;
+
if (isadd == REGISTER) {
for_each_cpu(cpu, mask) {
s = kmalloc_node(sizeof(struct listener),
@@ -631,11 +640,12 @@ void taskstats_exit(struct task_struct *tsk, int group_dead)
if (rc < 0)
return;
- stats = mk_reply(rep_skb, TASKSTATS_TYPE_PID, tsk->pid);
+ stats = mk_reply(rep_skb, TASKSTATS_TYPE_PID,
+ task_pid_nr_ns(tsk, &init_pid_ns));
if (!stats)
goto err;
- fill_stats(tsk, stats);
+ fill_stats(&init_user_ns, &init_pid_ns, tsk, stats);
/*
* Doesn't matter if tsk is the leader or the last group member leaving
@@ -643,7 +653,8 @@ void taskstats_exit(struct task_struct *tsk, int group_dead)
if (!is_thread_group || !group_dead)
goto send;
- stats = mk_reply(rep_skb, TASKSTATS_TYPE_TGID, tsk->tgid);
+ stats = mk_reply(rep_skb, TASKSTATS_TYPE_TGID,
+ task_tgid_nr_ns(tsk, &init_pid_ns));
if (!stats)
goto err;
diff --git a/kernel/tsacct.c b/kernel/tsacct.c
index 23b4d78..625df0b 100644
--- a/kernel/tsacct.c
+++ b/kernel/tsacct.c
@@ -26,7 +26,9 @@
/*
* fill in basic accounting fields
*/
-void bacct_add_tsk(struct taskstats *stats, struct task_struct *tsk)
+void bacct_add_tsk(struct user_namespace *user_ns,
+ struct pid_namespace *pid_ns,
+ struct taskstats *stats, struct task_struct *tsk)
{
const struct cred *tcred;
struct timespec uptime, ts;
@@ -55,13 +57,13 @@ void bacct_add_tsk(struct taskstats *stats, struct task_struct *tsk)
stats->ac_flag |= AXSIG;
stats->ac_nice = task_nice(tsk);
stats->ac_sched = tsk->policy;
- stats->ac_pid = tsk->pid;
+ stats->ac_pid = task_pid_nr_ns(tsk, pid_ns);
rcu_read_lock();
tcred = __task_cred(tsk);
- stats->ac_uid = tcred->uid;
- stats->ac_gid = tcred->gid;
+ stats->ac_uid = from_kuid_munged(user_ns, tcred->uid);
+ stats->ac_gid = from_kgid_munged(user_ns, tcred->gid);
stats->ac_ppid = pid_alive(tsk) ?
- rcu_dereference(tsk->real_parent)->tgid : 0;
+ task_tgid_nr_ns(rcu_dereference(tsk->real_parent), pid_ns) : 0;
rcu_read_unlock();
stats->ac_utime = cputime_to_usecs(tsk->utime);
stats->ac_stime = cputime_to_usecs(tsk->stime);
--
1.7.5.4
^ permalink raw reply related
* [REVIEW][PATCH 05/15] userns: Convert ipc to use kuid and kgid where appropriate
From: Eric W. Biederman @ 2012-08-26 0:00 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, linux-fsdevel, Serge E. Hallyn, David Miller
In-Reply-To: <87lih2h6i4.fsf@xmission.com>
- Store the ipc owner and creator with a kuid
- Store the ipc group and the crators group with a kgid.
- Add error handling to ipc_update_perms, allowing it to
fail if the uids and gids can not be converted to kuids
or kgids.
- Modify the proc files to display the ipc creator and
owner in the user namespace of the opener of the proc file.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
include/linux/ipc.h | 9 +++++----
init/Kconfig | 1 -
ipc/msg.c | 14 +++++++++-----
ipc/sem.c | 13 ++++++++-----
ipc/shm.c | 19 +++++++++++--------
ipc/util.c | 35 +++++++++++++++++++++--------------
ipc/util.h | 2 +-
7 files changed, 55 insertions(+), 38 deletions(-)
diff --git a/include/linux/ipc.h b/include/linux/ipc.h
index 30e8161..ca833fd 100644
--- a/include/linux/ipc.h
+++ b/include/linux/ipc.h
@@ -79,6 +79,7 @@ struct ipc_kludge {
#ifdef __KERNEL__
#include <linux/spinlock.h>
+#include <linux/uidgid.h>
#define IPCMNI 32768 /* <= MAX_INT limit for ipc arrays (including sysctl changes) */
@@ -89,10 +90,10 @@ struct kern_ipc_perm
int deleted;
int id;
key_t key;
- uid_t uid;
- gid_t gid;
- uid_t cuid;
- gid_t cgid;
+ kuid_t uid;
+ kgid_t gid;
+ kuid_t cuid;
+ kgid_t cgid;
umode_t mode;
unsigned long seq;
void *security;
diff --git a/init/Kconfig b/init/Kconfig
index 28715ec..4daf449 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -925,7 +925,6 @@ config UIDGID_CONVERTED
# List of kernel pieces that need user namespace work
# Features
- depends on SYSVIPC = n
depends on IMA = n
depends on EVM = n
depends on AUDIT = n
diff --git a/ipc/msg.c b/ipc/msg.c
index 7385de2..a71af5a 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -443,9 +443,12 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
goto out_unlock;
}
+ err = ipc_update_perm(&msqid64.msg_perm, ipcp);
+ if (err)
+ goto out_unlock;
+
msq->q_qbytes = msqid64.msg_qbytes;
- ipc_update_perm(&msqid64.msg_perm, ipcp);
msq->q_ctime = get_seconds();
/* sleeping receivers might be excluded by
* stricter permissions.
@@ -922,6 +925,7 @@ out:
#ifdef CONFIG_PROC_FS
static int sysvipc_msg_proc_show(struct seq_file *s, void *it)
{
+ struct user_namespace *user_ns = seq_user_ns(s);
struct msg_queue *msq = it;
return seq_printf(s,
@@ -933,10 +937,10 @@ static int sysvipc_msg_proc_show(struct seq_file *s, void *it)
msq->q_qnum,
msq->q_lspid,
msq->q_lrpid,
- msq->q_perm.uid,
- msq->q_perm.gid,
- msq->q_perm.cuid,
- msq->q_perm.cgid,
+ from_kuid_munged(user_ns, msq->q_perm.uid),
+ from_kgid_munged(user_ns, msq->q_perm.gid),
+ from_kuid_munged(user_ns, msq->q_perm.cuid),
+ from_kgid_munged(user_ns, msq->q_perm.cgid),
msq->q_stime,
msq->q_rtime,
msq->q_ctime);
diff --git a/ipc/sem.c b/ipc/sem.c
index 5215a81..58d31f1 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1104,7 +1104,9 @@ static int semctl_down(struct ipc_namespace *ns, int semid,
freeary(ns, ipcp);
goto out_up;
case IPC_SET:
- ipc_update_perm(&semid64.sem_perm, ipcp);
+ err = ipc_update_perm(&semid64.sem_perm, ipcp);
+ if (err)
+ goto out_unlock;
sma->sem_ctime = get_seconds();
break;
default:
@@ -1677,6 +1679,7 @@ void exit_sem(struct task_struct *tsk)
#ifdef CONFIG_PROC_FS
static int sysvipc_sem_proc_show(struct seq_file *s, void *it)
{
+ struct user_namespace *user_ns = seq_user_ns(s);
struct sem_array *sma = it;
return seq_printf(s,
@@ -1685,10 +1688,10 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it)
sma->sem_perm.id,
sma->sem_perm.mode,
sma->sem_nsems,
- sma->sem_perm.uid,
- sma->sem_perm.gid,
- sma->sem_perm.cuid,
- sma->sem_perm.cgid,
+ from_kuid_munged(user_ns, sma->sem_perm.uid),
+ from_kgid_munged(user_ns, sma->sem_perm.gid),
+ from_kuid_munged(user_ns, sma->sem_perm.cuid),
+ from_kgid_munged(user_ns, sma->sem_perm.cgid),
sma->sem_otime,
sma->sem_ctime);
}
diff --git a/ipc/shm.c b/ipc/shm.c
index 00faa05..dff40c9 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -758,7 +758,9 @@ static int shmctl_down(struct ipc_namespace *ns, int shmid, int cmd,
do_shm_rmid(ns, ipcp);
goto out_up;
case IPC_SET:
- ipc_update_perm(&shmid64.shm_perm, ipcp);
+ err = ipc_update_perm(&shmid64.shm_perm, ipcp);
+ if (err)
+ goto out_unlock;
shp->shm_ctim = get_seconds();
break;
default:
@@ -893,10 +895,10 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
audit_ipc_obj(&(shp->shm_perm));
if (!ns_capable(ns->user_ns, CAP_IPC_LOCK)) {
- uid_t euid = current_euid();
+ kuid_t euid = current_euid();
err = -EPERM;
- if (euid != shp->shm_perm.uid &&
- euid != shp->shm_perm.cuid)
+ if (!uid_eq(euid, shp->shm_perm.uid) &&
+ !uid_eq(euid, shp->shm_perm.cuid))
goto out_unlock;
if (cmd == SHM_LOCK && !rlimit(RLIMIT_MEMLOCK))
goto out_unlock;
@@ -1220,6 +1222,7 @@ SYSCALL_DEFINE1(shmdt, char __user *, shmaddr)
#ifdef CONFIG_PROC_FS
static int sysvipc_shm_proc_show(struct seq_file *s, void *it)
{
+ struct user_namespace *user_ns = seq_user_ns(s);
struct shmid_kernel *shp = it;
unsigned long rss = 0, swp = 0;
@@ -1242,10 +1245,10 @@ static int sysvipc_shm_proc_show(struct seq_file *s, void *it)
shp->shm_cprid,
shp->shm_lprid,
shp->shm_nattch,
- shp->shm_perm.uid,
- shp->shm_perm.gid,
- shp->shm_perm.cuid,
- shp->shm_perm.cgid,
+ from_kuid_munged(user_ns, shp->shm_perm.uid),
+ from_kgid_munged(user_ns, shp->shm_perm.gid),
+ from_kuid_munged(user_ns, shp->shm_perm.cuid),
+ from_kgid_munged(user_ns, shp->shm_perm.cgid),
shp->shm_atim,
shp->shm_dtim,
shp->shm_ctim,
diff --git a/ipc/util.c b/ipc/util.c
index eb07fd3..72fd078 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -249,8 +249,8 @@ int ipc_get_maxid(struct ipc_ids *ids)
int ipc_addid(struct ipc_ids* ids, struct kern_ipc_perm* new, int size)
{
- uid_t euid;
- gid_t egid;
+ kuid_t euid;
+ kgid_t egid;
int id, err;
if (size > IPCMNI)
@@ -606,14 +606,14 @@ void ipc_rcu_putref(void *ptr)
int ipcperms(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp, short flag)
{
- uid_t euid = current_euid();
+ kuid_t euid = current_euid();
int requested_mode, granted_mode;
audit_ipc_obj(ipcp);
requested_mode = (flag >> 6) | (flag >> 3) | flag;
granted_mode = ipcp->mode;
- if (euid == ipcp->cuid ||
- euid == ipcp->uid)
+ if (uid_eq(euid, ipcp->cuid) ||
+ uid_eq(euid, ipcp->uid))
granted_mode >>= 6;
else if (in_group_p(ipcp->cgid) || in_group_p(ipcp->gid))
granted_mode >>= 3;
@@ -643,10 +643,10 @@ int ipcperms(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp, short flag)
void kernel_to_ipc64_perm (struct kern_ipc_perm *in, struct ipc64_perm *out)
{
out->key = in->key;
- out->uid = in->uid;
- out->gid = in->gid;
- out->cuid = in->cuid;
- out->cgid = in->cgid;
+ out->uid = from_kuid_munged(current_user_ns(), in->uid);
+ out->gid = from_kgid_munged(current_user_ns(), in->gid);
+ out->cuid = from_kuid_munged(current_user_ns(), in->cuid);
+ out->cgid = from_kgid_munged(current_user_ns(), in->cgid);
out->mode = in->mode;
out->seq = in->seq;
}
@@ -747,12 +747,19 @@ int ipcget(struct ipc_namespace *ns, struct ipc_ids *ids,
* @in: the permission given as input.
* @out: the permission of the ipc to set.
*/
-void ipc_update_perm(struct ipc64_perm *in, struct kern_ipc_perm *out)
+int ipc_update_perm(struct ipc64_perm *in, struct kern_ipc_perm *out)
{
- out->uid = in->uid;
- out->gid = in->gid;
+ kuid_t uid = make_kuid(current_user_ns(), in->uid);
+ kgid_t gid = make_kgid(current_user_ns(), in->gid);
+ if (!uid_valid(uid) || !gid_valid(gid))
+ return -EINVAL;
+
+ out->uid = uid;
+ out->gid = gid;
out->mode = (out->mode & ~S_IRWXUGO)
| (in->mode & S_IRWXUGO);
+
+ return 0;
}
/**
@@ -777,7 +784,7 @@ struct kern_ipc_perm *ipcctl_pre_down(struct ipc_namespace *ns,
struct ipc64_perm *perm, int extra_perm)
{
struct kern_ipc_perm *ipcp;
- uid_t euid;
+ kuid_t euid;
int err;
down_write(&ids->rw_mutex);
@@ -793,7 +800,7 @@ struct kern_ipc_perm *ipcctl_pre_down(struct ipc_namespace *ns,
perm->gid, perm->mode);
euid = current_euid();
- if (euid == ipcp->cuid || euid == ipcp->uid ||
+ if (uid_eq(euid, ipcp->cuid) || uid_eq(euid, ipcp->uid) ||
ns_capable(ns->user_ns, CAP_SYS_ADMIN))
return ipcp;
diff --git a/ipc/util.h b/ipc/util.h
index 850ef3e..c8fe2f7 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -125,7 +125,7 @@ struct kern_ipc_perm *ipc_lock(struct ipc_ids *, int);
void kernel_to_ipc64_perm(struct kern_ipc_perm *in, struct ipc64_perm *out);
void ipc64_perm_to_ipc_perm(struct ipc64_perm *in, struct ipc_perm *out);
-void ipc_update_perm(struct ipc64_perm *in, struct kern_ipc_perm *out);
+int ipc_update_perm(struct ipc64_perm *in, struct kern_ipc_perm *out);
struct kern_ipc_perm *ipcctl_pre_down(struct ipc_namespace *ns,
struct ipc_ids *ids, int id, int cmd,
struct ipc64_perm *perm, int extra_perm);
--
1.7.5.4
^ permalink raw reply related
* [REVIEW][PATCH 04/15] userns: net: Call key_alloc with GLOBAL_ROOT_UID, GLOBAL_ROOT_GID instead of 0, 0
From: Eric W. Biederman @ 2012-08-26 0:00 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, linux-fsdevel, Serge E. Hallyn, David Miller, Sage Weil,
ceph-devel, David Howells, David Miller, linux-afs
In-Reply-To: <87lih2h6i4.fsf@xmission.com>
>From 088e7f2778bd228a4ce05f8dfaa4eb999e2207d2 Mon Sep 17 00:00:00 2001
From: "Eric W. Biederman" <ebiederm@xmission.com>
Date: Fri, 25 May 2012 16:37:54 -0600
Subject:
In net/dns_resolver/dns_key.c and net/rxrpc/ar-key.c make them
work with user namespaces enabled where key_alloc takes kuids and kgids.
Pass GLOBAL_ROOT_UID and GLOBAL_ROOT_GID instead of bare 0's.
Cc: Sage Weil <sage@inktank.com>
Cc: ceph-devel@vger.kernel.org
Cc: David Howells <dhowells@redhat.com>
Cc: David Miller <davem@davemloft.net>
Cc: linux-afs@lists.infradead.org
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
init/Kconfig | 2 --
net/dns_resolver/dns_key.c | 3 ++-
net/rxrpc/ar-key.c | 6 ++++--
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/init/Kconfig b/init/Kconfig
index d7f73bb..28715ec 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -941,8 +941,6 @@ config UIDGID_CONVERTED
# Networking
depends on NET_9P = n
- depends on AF_RXRPC = n
- depends on DNS_RESOLVER = n
# Filesystems
depends on USB_GADGETFS = n
diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index d9507dd..9807945 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -259,7 +259,8 @@ static int __init init_dns_resolver(void)
if (!cred)
return -ENOMEM;
- keyring = key_alloc(&key_type_keyring, ".dns_resolver", 0, 0, cred,
+ keyring = key_alloc(&key_type_keyring, ".dns_resolver",
+ GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, cred,
(KEY_POS_ALL & ~KEY_POS_SETATTR) |
KEY_USR_VIEW | KEY_USR_READ,
KEY_ALLOC_NOT_IN_QUOTA);
diff --git a/net/rxrpc/ar-key.c b/net/rxrpc/ar-key.c
index 8b1f9f4..011d238 100644
--- a/net/rxrpc/ar-key.c
+++ b/net/rxrpc/ar-key.c
@@ -948,7 +948,8 @@ int rxrpc_get_server_data_key(struct rxrpc_connection *conn,
_enter("");
- key = key_alloc(&key_type_rxrpc, "x", 0, 0, cred, 0,
+ key = key_alloc(&key_type_rxrpc, "x",
+ GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, cred, 0,
KEY_ALLOC_NOT_IN_QUOTA);
if (IS_ERR(key)) {
_leave(" = -ENOMEM [alloc %ld]", PTR_ERR(key));
@@ -994,7 +995,8 @@ struct key *rxrpc_get_null_key(const char *keyname)
struct key *key;
int ret;
- key = key_alloc(&key_type_rxrpc, keyname, 0, 0, cred,
+ key = key_alloc(&key_type_rxrpc, keyname,
+ GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, cred,
KEY_POS_SEARCH, KEY_ALLOC_NOT_IN_QUOTA);
if (IS_ERR(key))
return key;
--
1.7.5.4
^ permalink raw reply related
* [REVIEW][PATCH 03/15] userns: Convert security/keys to the new userns infrastructure
From: Eric W. Biederman @ 2012-08-25 23:59 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, linux-fsdevel, Serge E. Hallyn, David Miller,
linux-security-module, keyrings, David Howells
In-Reply-To: <87lih2h6i4.fsf@xmission.com>
- Replace key_user ->user_ns equality checks with kuid_has_mapping checks.
- Use from_kuid to generate key descriptions
- Use kuid_t and kgid_t and the associated helpers instead of uid_t and gid_t
- Avoid potential problems with file descriptor passing by displaying
keys in the user namespace of the opener of key status proc files.
Cc: linux-security-module@vger.kernel.org
Cc: keyrings@linux-nfs.org
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
include/linux/key.h | 9 ++++---
init/Kconfig | 1 -
security/keys/internal.h | 6 +---
security/keys/key.c | 23 ++++++------------
security/keys/keyctl.c | 50 +++++++++++++++++++++++++----------------
security/keys/keyring.c | 4 +-
security/keys/permission.c | 14 +++--------
security/keys/proc.c | 44 ++++++++++++++++++------------------
security/keys/process_keys.c | 15 ++++++------
security/keys/request_key.c | 6 ++--
10 files changed, 84 insertions(+), 88 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index cef3b31..2393b1c 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -24,6 +24,7 @@
#include <linux/atomic.h>
#ifdef __KERNEL__
+#include <linux/uidgid.h>
/* key handle serial number */
typedef int32_t key_serial_t;
@@ -137,8 +138,8 @@ struct key {
time_t revoked_at; /* time at which key was revoked */
};
time_t last_used_at; /* last time used for LRU keyring discard */
- uid_t uid;
- gid_t gid;
+ kuid_t uid;
+ kgid_t gid;
key_perm_t perm; /* access permissions */
unsigned short quotalen; /* length added to quota */
unsigned short datalen; /* payload data length
@@ -193,7 +194,7 @@ struct key {
extern struct key *key_alloc(struct key_type *type,
const char *desc,
- uid_t uid, gid_t gid,
+ kuid_t uid, kgid_t gid,
const struct cred *cred,
key_perm_t perm,
unsigned long flags);
@@ -262,7 +263,7 @@ extern int key_link(struct key *keyring,
extern int key_unlink(struct key *keyring,
struct key *key);
-extern struct key *keyring_alloc(const char *description, uid_t uid, gid_t gid,
+extern struct key *keyring_alloc(const char *description, kuid_t uid, kgid_t gid,
const struct cred *cred,
unsigned long flags,
struct key *dest);
diff --git a/init/Kconfig b/init/Kconfig
index fdabc51..d7f73bb 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -928,7 +928,6 @@ config UIDGID_CONVERTED
depends on SYSVIPC = n
depends on IMA = n
depends on EVM = n
- depends on KEYS = n
depends on AUDIT = n
depends on AUDITSYSCALL = n
depends on TASKSTATS = n
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 22ff052..8bbefc3 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -52,8 +52,7 @@ struct key_user {
atomic_t usage; /* for accessing qnkeys & qnbytes */
atomic_t nkeys; /* number of keys */
atomic_t nikeys; /* number of instantiated keys */
- uid_t uid;
- struct user_namespace *user_ns;
+ kuid_t uid;
int qnkeys; /* number of keys allocated to this user */
int qnbytes; /* number of bytes allocated to this user */
};
@@ -62,8 +61,7 @@ extern struct rb_root key_user_tree;
extern spinlock_t key_user_lock;
extern struct key_user root_key_user;
-extern struct key_user *key_user_lookup(uid_t uid,
- struct user_namespace *user_ns);
+extern struct key_user *key_user_lookup(kuid_t uid);
extern void key_user_put(struct key_user *user);
/*
diff --git a/security/keys/key.c b/security/keys/key.c
index 50d96d4..4289c5b 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -18,7 +18,6 @@
#include <linux/workqueue.h>
#include <linux/random.h>
#include <linux/err.h>
-#include <linux/user_namespace.h>
#include "internal.h"
struct kmem_cache *key_jar;
@@ -52,7 +51,7 @@ void __key_check(const struct key *key)
* Get the key quota record for a user, allocating a new record if one doesn't
* already exist.
*/
-struct key_user *key_user_lookup(uid_t uid, struct user_namespace *user_ns)
+struct key_user *key_user_lookup(kuid_t uid)
{
struct key_user *candidate = NULL, *user;
struct rb_node *parent = NULL;
@@ -67,13 +66,9 @@ try_again:
parent = *p;
user = rb_entry(parent, struct key_user, node);
- if (uid < user->uid)
+ if (uid_lt(uid, user->uid))
p = &(*p)->rb_left;
- else if (uid > user->uid)
- p = &(*p)->rb_right;
- else if (user_ns < user->user_ns)
- p = &(*p)->rb_left;
- else if (user_ns > user->user_ns)
+ else if (uid_gt(uid, user->uid))
p = &(*p)->rb_right;
else
goto found;
@@ -102,7 +97,6 @@ try_again:
atomic_set(&candidate->nkeys, 0);
atomic_set(&candidate->nikeys, 0);
candidate->uid = uid;
- candidate->user_ns = get_user_ns(user_ns);
candidate->qnkeys = 0;
candidate->qnbytes = 0;
spin_lock_init(&candidate->lock);
@@ -131,7 +125,6 @@ void key_user_put(struct key_user *user)
if (atomic_dec_and_lock(&user->usage, &key_user_lock)) {
rb_erase(&user->node, &key_user_tree);
spin_unlock(&key_user_lock);
- put_user_ns(user->user_ns);
kfree(user);
}
@@ -229,7 +222,7 @@ serial_exists:
* key_alloc() calls don't race with module unloading.
*/
struct key *key_alloc(struct key_type *type, const char *desc,
- uid_t uid, gid_t gid, const struct cred *cred,
+ kuid_t uid, kgid_t gid, const struct cred *cred,
key_perm_t perm, unsigned long flags)
{
struct key_user *user = NULL;
@@ -253,16 +246,16 @@ struct key *key_alloc(struct key_type *type, const char *desc,
quotalen = desclen + type->def_datalen;
/* get hold of the key tracking for this user */
- user = key_user_lookup(uid, cred->user_ns);
+ user = key_user_lookup(uid);
if (!user)
goto no_memory_1;
/* check that the user's quota permits allocation of another key and
* its description */
if (!(flags & KEY_ALLOC_NOT_IN_QUOTA)) {
- unsigned maxkeys = (uid == 0) ?
+ unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ?
key_quota_root_maxkeys : key_quota_maxkeys;
- unsigned maxbytes = (uid == 0) ?
+ unsigned maxbytes = uid_eq(uid, GLOBAL_ROOT_UID) ?
key_quota_root_maxbytes : key_quota_maxbytes;
spin_lock(&user->lock);
@@ -380,7 +373,7 @@ int key_payload_reserve(struct key *key, size_t datalen)
/* contemplate the quota adjustment */
if (delta != 0 && test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
- unsigned maxbytes = (key->user->uid == 0) ?
+ unsigned maxbytes = uid_eq(key->user->uid, GLOBAL_ROOT_UID) ?
key_quota_root_maxbytes : key_quota_maxbytes;
spin_lock(&key->user->lock);
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 3364fbf..1ecc0f7 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -569,8 +569,8 @@ okay:
ret = snprintf(tmpbuf, PAGE_SIZE - 1,
"%s;%d;%d;%08x;%s",
key->type->name,
- key->uid,
- key->gid,
+ from_kuid_munged(current_user_ns(), key->uid),
+ from_kgid_munged(current_user_ns(), key->gid),
key->perm,
key->description ?: "");
@@ -766,15 +766,25 @@ error:
*
* If successful, 0 will be returned.
*/
-long keyctl_chown_key(key_serial_t id, uid_t uid, gid_t gid)
+long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
{
struct key_user *newowner, *zapowner = NULL;
struct key *key;
key_ref_t key_ref;
long ret;
+ kuid_t uid;
+ kgid_t gid;
+
+ uid = make_kuid(current_user_ns(), user);
+ gid = make_kgid(current_user_ns(), group);
+ ret = -EINVAL;
+ if ((user != (uid_t) -1) && !uid_valid(uid))
+ goto error;
+ if ((group != (gid_t) -1) && !gid_valid(gid))
+ goto error;
ret = 0;
- if (uid == (uid_t) -1 && gid == (gid_t) -1)
+ if (user == (uid_t) -1 && group == (gid_t) -1)
goto error;
key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL,
@@ -792,27 +802,27 @@ long keyctl_chown_key(key_serial_t id, uid_t uid, gid_t gid)
if (!capable(CAP_SYS_ADMIN)) {
/* only the sysadmin can chown a key to some other UID */
- if (uid != (uid_t) -1 && key->uid != uid)
+ if (user != (uid_t) -1 && !uid_eq(key->uid, uid))
goto error_put;
/* only the sysadmin can set the key's GID to a group other
* than one of those that the current process subscribes to */
- if (gid != (gid_t) -1 && gid != key->gid && !in_group_p(gid))
+ if (group != (gid_t) -1 && !gid_eq(gid, key->gid) && !in_group_p(gid))
goto error_put;
}
/* change the UID */
- if (uid != (uid_t) -1 && uid != key->uid) {
+ if (user != (uid_t) -1 && !uid_eq(uid, key->uid)) {
ret = -ENOMEM;
- newowner = key_user_lookup(uid, current_user_ns());
+ newowner = key_user_lookup(uid);
if (!newowner)
goto error_put;
/* transfer the quota burden to the new user */
if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
- unsigned maxkeys = (uid == 0) ?
+ unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ?
key_quota_root_maxkeys : key_quota_maxkeys;
- unsigned maxbytes = (uid == 0) ?
+ unsigned maxbytes = uid_eq(uid, GLOBAL_ROOT_UID) ?
key_quota_root_maxbytes : key_quota_maxbytes;
spin_lock(&newowner->lock);
@@ -846,7 +856,7 @@ long keyctl_chown_key(key_serial_t id, uid_t uid, gid_t gid)
}
/* change the GID */
- if (gid != (gid_t) -1)
+ if (group != (gid_t) -1)
key->gid = gid;
ret = 0;
@@ -897,7 +907,7 @@ long keyctl_setperm_key(key_serial_t id, key_perm_t perm)
down_write(&key->sem);
/* if we're not the sysadmin, we can only change a key that we own */
- if (capable(CAP_SYS_ADMIN) || key->uid == current_fsuid()) {
+ if (capable(CAP_SYS_ADMIN) || uid_eq(key->uid, current_fsuid())) {
key->perm = perm;
ret = 0;
}
@@ -1507,18 +1517,18 @@ long keyctl_session_to_parent(void)
/* the parent must have the same effective ownership and mustn't be
* SUID/SGID */
- if (pcred->uid != mycred->euid ||
- pcred->euid != mycred->euid ||
- pcred->suid != mycred->euid ||
- pcred->gid != mycred->egid ||
- pcred->egid != mycred->egid ||
- pcred->sgid != mycred->egid)
+ if (!uid_eq(pcred->uid, mycred->euid) ||
+ !uid_eq(pcred->euid, mycred->euid) ||
+ !uid_eq(pcred->suid, mycred->euid) ||
+ !gid_eq(pcred->gid, mycred->egid) ||
+ !gid_eq(pcred->egid, mycred->egid) ||
+ !gid_eq(pcred->sgid, mycred->egid))
goto unlock;
/* the keyrings must have the same UID */
if ((pcred->tgcred->session_keyring &&
- pcred->tgcred->session_keyring->uid != mycred->euid) ||
- mycred->tgcred->session_keyring->uid != mycred->euid)
+ !uid_eq(pcred->tgcred->session_keyring->uid, mycred->euid)) ||
+ !uid_eq(mycred->tgcred->session_keyring->uid, mycred->euid))
goto unlock;
/* cancel an already pending keyring replacement */
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 81e7852..a5f5c4b 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -256,7 +256,7 @@ error:
/*
* Allocate a keyring and link into the destination keyring.
*/
-struct key *keyring_alloc(const char *description, uid_t uid, gid_t gid,
+struct key *keyring_alloc(const char *description, kuid_t uid, kgid_t gid,
const struct cred *cred, unsigned long flags,
struct key *dest)
{
@@ -612,7 +612,7 @@ struct key *find_keyring_by_name(const char *name, bool skip_perm_check)
&keyring_name_hash[bucket],
type_data.link
) {
- if (keyring->user->user_ns != current_user_ns())
+ if (!kuid_has_mapping(current_user_ns(), keyring->user->uid))
continue;
if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
diff --git a/security/keys/permission.c b/security/keys/permission.c
index 0b4d019..efcc0c8 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -36,33 +36,27 @@ int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
key = key_ref_to_ptr(key_ref);
- if (key->user->user_ns != cred->user_ns)
- goto use_other_perms;
-
/* use the second 8-bits of permissions for keys the caller owns */
- if (key->uid == cred->fsuid) {
+ if (uid_eq(key->uid, cred->fsuid)) {
kperm = key->perm >> 16;
goto use_these_perms;
}
/* use the third 8-bits of permissions for keys the caller has a group
* membership in common with */
- if (key->gid != -1 && key->perm & KEY_GRP_ALL) {
- if (key->gid == cred->fsgid) {
+ if (gid_valid(key->gid) && key->perm & KEY_GRP_ALL) {
+ if (gid_eq(key->gid, cred->fsgid)) {
kperm = key->perm >> 8;
goto use_these_perms;
}
- ret = groups_search(cred->group_info,
- make_kgid(current_user_ns(), key->gid));
+ ret = groups_search(cred->group_info, key->gid);
if (ret) {
kperm = key->perm >> 8;
goto use_these_perms;
}
}
-use_other_perms:
-
/* otherwise use the least-significant 8-bits */
kperm = key->perm;
diff --git a/security/keys/proc.c b/security/keys/proc.c
index 30d1ddf..217b685 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -88,14 +88,14 @@ __initcall(key_proc_init);
*/
#ifdef CONFIG_KEYS_DEBUG_PROC_KEYS
-static struct rb_node *key_serial_next(struct rb_node *n)
+static struct rb_node *key_serial_next(struct seq_file *p, struct rb_node *n)
{
- struct user_namespace *user_ns = current_user_ns();
+ struct user_namespace *user_ns = seq_user_ns(p);
n = rb_next(n);
while (n) {
struct key *key = rb_entry(n, struct key, serial_node);
- if (key->user->user_ns == user_ns)
+ if (kuid_has_mapping(user_ns, key->user->uid))
break;
n = rb_next(n);
}
@@ -107,9 +107,9 @@ static int proc_keys_open(struct inode *inode, struct file *file)
return seq_open(file, &proc_keys_ops);
}
-static struct key *find_ge_key(key_serial_t id)
+static struct key *find_ge_key(struct seq_file *p, key_serial_t id)
{
- struct user_namespace *user_ns = current_user_ns();
+ struct user_namespace *user_ns = seq_user_ns(p);
struct rb_node *n = key_serial_tree.rb_node;
struct key *minkey = NULL;
@@ -132,7 +132,7 @@ static struct key *find_ge_key(key_serial_t id)
return NULL;
for (;;) {
- if (minkey->user->user_ns == user_ns)
+ if (kuid_has_mapping(user_ns, minkey->user->uid))
return minkey;
n = rb_next(&minkey->serial_node);
if (!n)
@@ -151,7 +151,7 @@ static void *proc_keys_start(struct seq_file *p, loff_t *_pos)
if (*_pos > INT_MAX)
return NULL;
- key = find_ge_key(pos);
+ key = find_ge_key(p, pos);
if (!key)
return NULL;
*_pos = key->serial;
@@ -168,7 +168,7 @@ static void *proc_keys_next(struct seq_file *p, void *v, loff_t *_pos)
{
struct rb_node *n;
- n = key_serial_next(v);
+ n = key_serial_next(p, v);
if (n)
*_pos = key_node_serial(n);
return n;
@@ -254,8 +254,8 @@ static int proc_keys_show(struct seq_file *m, void *v)
atomic_read(&key->usage),
xbuf,
key->perm,
- key->uid,
- key->gid,
+ from_kuid_munged(seq_user_ns(m), key->uid),
+ from_kgid_munged(seq_user_ns(m), key->gid),
key->type->name);
#undef showflag
@@ -270,26 +270,26 @@ static int proc_keys_show(struct seq_file *m, void *v)
#endif /* CONFIG_KEYS_DEBUG_PROC_KEYS */
-static struct rb_node *__key_user_next(struct rb_node *n)
+static struct rb_node *__key_user_next(struct user_namespace *user_ns, struct rb_node *n)
{
while (n) {
struct key_user *user = rb_entry(n, struct key_user, node);
- if (user->user_ns == current_user_ns())
+ if (kuid_has_mapping(user_ns, user->uid))
break;
n = rb_next(n);
}
return n;
}
-static struct rb_node *key_user_next(struct rb_node *n)
+static struct rb_node *key_user_next(struct user_namespace *user_ns, struct rb_node *n)
{
- return __key_user_next(rb_next(n));
+ return __key_user_next(user_ns, rb_next(n));
}
-static struct rb_node *key_user_first(struct rb_root *r)
+static struct rb_node *key_user_first(struct user_namespace *user_ns, struct rb_root *r)
{
struct rb_node *n = rb_first(r);
- return __key_user_next(n);
+ return __key_user_next(user_ns, n);
}
/*
@@ -309,10 +309,10 @@ static void *proc_key_users_start(struct seq_file *p, loff_t *_pos)
spin_lock(&key_user_lock);
- _p = key_user_first(&key_user_tree);
+ _p = key_user_first(seq_user_ns(p), &key_user_tree);
while (pos > 0 && _p) {
pos--;
- _p = key_user_next(_p);
+ _p = key_user_next(seq_user_ns(p), _p);
}
return _p;
@@ -321,7 +321,7 @@ static void *proc_key_users_start(struct seq_file *p, loff_t *_pos)
static void *proc_key_users_next(struct seq_file *p, void *v, loff_t *_pos)
{
(*_pos)++;
- return key_user_next((struct rb_node *)v);
+ return key_user_next(seq_user_ns(p), (struct rb_node *)v);
}
static void proc_key_users_stop(struct seq_file *p, void *v)
@@ -334,13 +334,13 @@ static int proc_key_users_show(struct seq_file *m, void *v)
{
struct rb_node *_p = v;
struct key_user *user = rb_entry(_p, struct key_user, node);
- unsigned maxkeys = (user->uid == 0) ?
+ unsigned maxkeys = uid_eq(user->uid, GLOBAL_ROOT_UID) ?
key_quota_root_maxkeys : key_quota_maxkeys;
- unsigned maxbytes = (user->uid == 0) ?
+ unsigned maxbytes = uid_eq(user->uid, GLOBAL_ROOT_UID) ?
key_quota_root_maxbytes : key_quota_maxbytes;
seq_printf(m, "%5u: %5d %d/%d %d/%d %d/%d\n",
- user->uid,
+ from_kuid_munged(seq_user_ns(m), user->uid),
atomic_read(&user->usage),
atomic_read(&user->nkeys),
atomic_read(&user->nikeys),
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 54339cf..a58f712 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -34,8 +34,7 @@ struct key_user root_key_user = {
.lock = __SPIN_LOCK_UNLOCKED(root_key_user.lock),
.nkeys = ATOMIC_INIT(2),
.nikeys = ATOMIC_INIT(2),
- .uid = 0,
- .user_ns = &init_user_ns,
+ .uid = GLOBAL_ROOT_UID,
};
/*
@@ -48,11 +47,13 @@ int install_user_keyrings(void)
struct key *uid_keyring, *session_keyring;
char buf[20];
int ret;
+ uid_t uid;
cred = current_cred();
user = cred->user;
+ uid = from_kuid(cred->user_ns, user->uid);
- kenter("%p{%u}", user, user->uid);
+ kenter("%p{%u}", user, uid);
if (user->uid_keyring) {
kleave(" = 0 [exist]");
@@ -67,11 +68,11 @@ int install_user_keyrings(void)
* - there may be one in existence already as it may have been
* pinned by a session, but the user_struct pointing to it
* may have been destroyed by setuid */
- sprintf(buf, "_uid.%u", user->uid);
+ sprintf(buf, "_uid.%u", uid);
uid_keyring = find_keyring_by_name(buf, true);
if (IS_ERR(uid_keyring)) {
- uid_keyring = keyring_alloc(buf, user->uid, (gid_t) -1,
+ uid_keyring = keyring_alloc(buf, user->uid, INVALID_GID,
cred, KEY_ALLOC_IN_QUOTA,
NULL);
if (IS_ERR(uid_keyring)) {
@@ -82,12 +83,12 @@ int install_user_keyrings(void)
/* get a default session keyring (which might also exist
* already) */
- sprintf(buf, "_uid_ses.%u", user->uid);
+ sprintf(buf, "_uid_ses.%u", uid);
session_keyring = find_keyring_by_name(buf, true);
if (IS_ERR(session_keyring)) {
session_keyring =
- keyring_alloc(buf, user->uid, (gid_t) -1,
+ keyring_alloc(buf, user->uid, INVALID_GID,
cred, KEY_ALLOC_IN_QUOTA, NULL);
if (IS_ERR(session_keyring)) {
ret = PTR_ERR(session_keyring);
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 000e750..66e2118 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -139,8 +139,8 @@ static int call_sbin_request_key(struct key_construction *cons,
goto error_link;
/* record the UID and GID */
- sprintf(uid_str, "%d", cred->fsuid);
- sprintf(gid_str, "%d", cred->fsgid);
+ sprintf(uid_str, "%d", from_kuid(&init_user_ns, cred->fsuid));
+ sprintf(gid_str, "%d", from_kgid(&init_user_ns, cred->fsgid));
/* we say which key is under construction */
sprintf(key_str, "%d", key->serial);
@@ -442,7 +442,7 @@ static struct key *construct_key_and_link(struct key_type *type,
kenter("");
- user = key_user_lookup(current_fsuid(), current_user_ns());
+ user = key_user_lookup(current_fsuid());
if (!user)
return ERR_PTR(-ENOMEM);
--
1.7.5.4
^ permalink raw reply related
* [REVIEW][PATCH 02/15] userns: Make credential debugging user namespace safe.
From: Eric W. Biederman @ 2012-08-25 23:59 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, linux-fsdevel, Serge E. Hallyn, David Miller,
David Howells
In-Reply-To: <87lih2h6i4.fsf@xmission.com>
Cc: David Howells <dhowells@redhat.com>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
init/Kconfig | 1 -
kernel/cred.c | 10 ++++++++--
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/init/Kconfig b/init/Kconfig
index 448b701..fdabc51 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -936,7 +936,6 @@ config UIDGID_CONVERTED
depends on FS_POSIX_ACL = n
depends on QUOTA = n
depends on QUOTACTL = n
- depends on DEBUG_CREDENTIALS = n
depends on BSD_PROCESS_ACCT = n
depends on DRM = n
depends on PROC_EVENTS = n
diff --git a/kernel/cred.c b/kernel/cred.c
index de728ac..48cea3d 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -799,9 +799,15 @@ static void dump_invalid_creds(const struct cred *cred, const char *label,
atomic_read(&cred->usage),
read_cred_subscribers(cred));
printk(KERN_ERR "CRED: ->*uid = { %d,%d,%d,%d }\n",
- cred->uid, cred->euid, cred->suid, cred->fsuid);
+ from_kuid_munged(&init_user_ns, cred->uid),
+ from_kuid_munged(&init_user_ns, cred->euid),
+ from_kuid_munged(&init_user_ns, cred->suid),
+ from_kuid_munged(&init_user_ns, cred->fsuid));
printk(KERN_ERR "CRED: ->*gid = { %d,%d,%d,%d }\n",
- cred->gid, cred->egid, cred->sgid, cred->fsgid);
+ from_kgid_munged(&init_user_ns, cred->gid),
+ from_kgid_munged(&init_user_ns, cred->egid),
+ from_kgid_munged(&init_user_ns, cred->sgid),
+ from_kgid_munged(&init_user_ns, cred->fsgid));
#ifdef CONFIG_SECURITY
printk(KERN_ERR "CRED: ->security is %p\n", cred->security);
if ((unsigned long) cred->security >= PAGE_SIZE &&
--
1.7.5.4
^ permalink raw reply related
* [REVIEW][PATCH 01/15] userns: Enable building of pf_key sockets when user namespace support is enabled.
From: Eric W. Biederman @ 2012-08-25 23:58 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, linux-fsdevel, Serge E. Hallyn, David Miller
In-Reply-To: <87lih2h6i4.fsf@xmission.com>
Enable building of pf_key sockets and user namespace support at the
same time. This combination builds successfully so there is no reason
to forbid it.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
init/Kconfig | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/init/Kconfig b/init/Kconfig
index b445d6f..448b701 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -944,7 +944,6 @@ config UIDGID_CONVERTED
# Networking
depends on NET_9P = n
depends on AF_RXRPC = n
- depends on NET_KEY = n
depends on DNS_RESOLVER = n
# Filesystems
--
1.7.5.4
^ permalink raw reply related
* [REVIEW][PATCH 0/15] userns subsystem conversions
From: Eric W. Biederman @ 2012-08-25 23:54 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, linux-fsdevel, Serge E. Hallyn, David Miller
This patchset updates all of the major linux subsystems that use uids
and gids to store them in kuid_t and kgid_t types.
This update allows some of the subsystems to work in all user namespaces
while other subsystems were updated to only work in the initial user
namespace.
kuid_t and kgid_t values have been pushed as deeply into the code as
possible to allow type checking to find as many problems as possible.
In a couple of cases this involved taking an implicit union stored in
an unsigned int and making it an explicit union.
This patchset is based on 3.6-rc1 and strictly against:
git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-next
My intention after these patches have been reviewed is to add them to my
non-rebasing for-next branch of my user namespace tree and to merge
these changes into 3.7.
I had hoped when I converted the core kernel that I would have removed
the interactions between subsystems and would be able to merge these
changes independently through maintainer trees in a timely fashion, but
there are just enough dependencies and interactions that the changes
really all need to be in one tree to make these changes testable/usable.
Once these changes hit my for-next branch I won't be rebasing them so
if a maintainer wants to merge them to avoid conflicts feel free.
The biggest cross subystem change this round is probably the change
to have audit_get_loginuid return a kuid_t, but it certainly isn't
the only cross subsystem change.
Eric W. Biederman (15):
userns: Enable building of pf_key sockets when user namespace support is enabled.
userns: Make credential debugging user namespace safe.
userns: Convert security/keys to the new userns infrastructure
userns: net: Call key_alloc with GLOBAL_ROOT_UID, GLOBAL_ROOT_GID instead of 0, 0
userns: Convert ipc to use kuid and kgid where appropriate
userns: Convert audit to use kuid and kgid where appropriate
userns: Convert taskstats to handle the user and pid namespaces.
userns: Convert bsd process accounting to use kuid and kgid where appropriate
userns: Convert process event connector to handle kuids and kgids
userns: Convert debugfs to use kuid/kgid where appropriate.
userns: Teach trace to use from_kuid
userns: Convert drm to use kuid and kgid and struct pid where appropriate
userns: Add basic quota support
userns: Convert vfs posix_acl support to use kuid and kgid where appripriate.
userns: Convert configfs to use kuid and kgid where appropriate
drivers/connector/cn_proc.c | 18 +++-
drivers/gpu/drm/drm_fops.c | 3 +-
drivers/gpu/drm/drm_info.c | 5 +-
drivers/gpu/drm/drm_ioctl.c | 4 +-
drivers/tty/tty_audit.c | 16 ++--
fs/9p/acl.c | 8 +-
fs/btrfs/acl.c | 8 +-
fs/configfs/inode.c | 4 +-
fs/debugfs/inode.c | 26 +++--
fs/ext2/acl.c | 32 ++++--
fs/ext3/acl.c | 32 ++++--
fs/ext4/acl.c | 31 ++++--
fs/generic_acl.c | 4 +-
fs/gfs2/acl.c | 14 ++--
fs/gfs2/quota.c | 44 +++++---
fs/jffs2/acl.c | 4 +-
fs/jfs/acl.c | 4 +-
fs/jfs/xattr.c | 4 +-
fs/nfs/nfs3acl.c | 4 +-
fs/nfsd/vfs.c | 8 +-
fs/ocfs2/acl.c | 4 +-
fs/ocfs2/file.c | 6 +-
fs/ocfs2/quota_global.c | 34 +++++--
fs/ocfs2/quota_local.c | 12 ++-
fs/posix_acl.c | 30 +++---
fs/proc/base.c | 12 ++-
fs/quota/dquot.c | 43 ++++----
fs/quota/netlink.c | 11 ++-
fs/quota/quota.c | 44 +++++---
fs/quota/quota_tree.c | 20 +++-
fs/quota/quota_v1.c | 8 +-
fs/quota/quota_v2.c | 14 ++-
drivers/connector/cn_proc.c | 18 +++-
drivers/gpu/drm/drm_fops.c | 3 +-
drivers/gpu/drm/drm_info.c | 5 +-
drivers/gpu/drm/drm_ioctl.c | 4 +-
drivers/tty/tty_audit.c | 16 ++--
fs/9p/acl.c | 8 +-
fs/btrfs/acl.c | 8 +-
fs/configfs/inode.c | 4 +-
fs/debugfs/inode.c | 26 +++--
fs/ext2/acl.c | 32 ++++--
fs/ext3/acl.c | 32 ++++--
fs/ext4/acl.c | 31 ++++--
fs/generic_acl.c | 4 +-
fs/gfs2/acl.c | 14 ++--
fs/gfs2/quota.c | 44 +++++---
fs/jffs2/acl.c | 4 +-
fs/jfs/acl.c | 4 +-
fs/jfs/xattr.c | 4 +-
fs/nfs/nfs3acl.c | 4 +-
fs/nfsd/vfs.c | 8 +-
fs/ocfs2/acl.c | 4 +-
fs/ocfs2/file.c | 6 +-
fs/ocfs2/quota_global.c | 34 +++++--
fs/ocfs2/quota_local.c | 12 ++-
fs/posix_acl.c | 30 +++---
fs/proc/base.c | 12 ++-
fs/quota/dquot.c | 43 ++++----
fs/quota/netlink.c | 11 ++-
fs/quota/quota.c | 44 +++++---
fs/quota/quota_tree.c | 20 +++-
fs/quota/quota_v1.c | 8 +-
fs/quota/quota_v2.c | 14 ++-
fs/reiserfs/xattr_acl.c | 4 +-
fs/xattr.c | 7 ++
fs/xattr_acl.c | 96 +++++++++++++++--
fs/xfs/xfs_acl.c | 4 +-
fs/xfs/xfs_quotaops.c | 18 ++--
fs/xfs/xfs_trans_dquot.c | 8 +-
include/drm/drmP.h | 4 +-
include/linux/audit.h | 12 ++-
include/linux/init_task.h | 2 +-
include/linux/ipc.h | 9 +-
include/linux/key.h | 9 +-
include/linux/posix_acl.h | 8 ++-
include/linux/posix_acl_xattr.h | 18 +++-
include/linux/quota.h | 91 +++++++++++++++-
include/linux/quotaops.h | 18 +++-
include/linux/sched.h | 2 +-
include/linux/tsacct_kern.h | 8 +-
include/linux/tty.h | 4 +-
include/net/netlabel.h | 2 +-
include/net/xfrm.h | 23 ++--
init/Kconfig | 18 ---
ipc/msg.c | 14 ++-
ipc/sem.c | 13 ++-
ipc/shm.c | 19 ++--
ipc/util.c | 35 ++++---
ipc/util.h | 2 +-
kernel/acct.c | 4 +-
kernel/audit.c | 42 +++++---
kernel/audit.h | 4 +-
kernel/audit_watch.c | 2 +-
kernel/auditfilter.c | 142 +++++++++++++++++++++----
kernel/auditsc.c | 214 +++++++++++++++++++------------------
kernel/cred.c | 10 ++-
kernel/taskstats.c | 23 +++-
kernel/trace/trace.c | 3 +-
kernel/trace/trace.h | 2 +-
kernel/tsacct.c | 12 ++-
net/core/dev.c | 2 +-
net/dns_resolver/dns_key.c | 3 +-
net/netlabel/netlabel_unlabeled.c | 2 +-
net/netlabel/netlabel_user.c | 2 +-
net/rxrpc/ar-key.c | 6 +-
net/xfrm/xfrm_policy.c | 8 +-
net/xfrm/xfrm_state.c | 6 +-
net/xfrm/xfrm_user.c | 12 +-
security/keys/internal.h | 6 +-
security/keys/key.c | 23 ++---
security/keys/keyctl.c | 50 +++++----
security/keys/keyring.c | 4 +-
security/keys/permission.c | 14 +--
security/keys/proc.c | 44 ++++----
security/keys/process_keys.c | 15 ++--
security/keys/request_key.c | 6 +-
85 files changed, 1056 insertions(+), 564 deletions(-)
^ permalink raw reply
* Re: [PATCH 0/8] csiostor: Chelsio FCoE offload driver submission
From: Naresh Kumar Inna @ 2012-08-25 19:08 UTC (permalink / raw)
To: David Miller
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <20120824.151008.248026066556198861.davem@davemloft.net>
On 8/25/2012 12:40 AM, David Miller wrote:
> From: Naresh Kumar Inna <naresh@chelsio.com>
> Date: Sat, 25 Aug 2012 00:34:35 +0530
>
>> Thanks for reviewing. Is your comment with respect to any particular
>> module parameter[s] in this driver or all of them?
>
> All of them.
>
So are module parameters being obsoleted? You mentioned about using
generic kernel facilities for tuning driver knobs. Could you please
elaborate a bit more?
Thanks,
Naresh.
^ permalink raw reply
* Re: [PATCH 6/8] csiostor: Chelsio FCoE offload driver submission (headers part 1).
From: Naresh Kumar Inna @ 2012-08-25 19:01 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <1345920029.28432.102.camel@haakon2.linux-iscsi.org>
On 8/26/2012 12:10 AM, Nicholas A. Bellinger wrote:
> On Sat, 2012-08-25 at 23:39 +0530, Naresh Kumar Inna wrote:
>> On 8/25/2012 2:47 AM, Nicholas A. Bellinger wrote:
>>> On Sat, 2012-08-25 at 00:06 +0530, Naresh Kumar Inna wrote:
>>>> On 8/24/2012 1:28 AM, Nicholas A. Bellinger wrote:
>>>>> On Fri, 2012-08-24 at 03:57 +0530, Naresh Kumar Inna wrote:
>>>>>> This patch contains the first set of the header files for csiostor driver.
>>>>>>
>>>>>> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
>>>>>> ---
>>>>>> drivers/scsi/csiostor/csio_defs.h | 143 ++++++
>>>>>> drivers/scsi/csiostor/csio_fcoe_proto.h | 843 +++++++++++++++++++++++++++++++
>>>>>> drivers/scsi/csiostor/csio_hw.h | 668 ++++++++++++++++++++++++
>>>>>> drivers/scsi/csiostor/csio_init.h | 158 ++++++
>>>>>> 4 files changed, 1812 insertions(+), 0 deletions(-)
>>>>>> create mode 100644 drivers/scsi/csiostor/csio_defs.h
>>>>>> create mode 100644 drivers/scsi/csiostor/csio_fcoe_proto.h
>>>>>> create mode 100644 drivers/scsi/csiostor/csio_hw.h
>>>>>> create mode 100644 drivers/scsi/csiostor/csio_init.h
>>>>>>
>>>>>
>>>>> Hi Naresh,
>>>>>
>>>>> Just commenting on csio_defs.h bits here... As Robert mentioned, you'll
>>>>> need to convert the driver to use (or add to) upstream protocol
>>>>> definitions and drop the csio_fcoe_proto.h bits..
>>>>>
>>>>
>>>> Hi Nicholas,
>>>>
>>>> I would like take up the discussion of the protocol header file in that
>>>> email thread. Please find the rest of my replies inline.
>>>>
>>>> Thanks for reviewing,
>>>> Naresh.
>>>>
>>>>>> diff --git a/drivers/scsi/csiostor/csio_defs.h b/drivers/scsi/csiostor/csio_defs.h
>>>>>> new file mode 100644
>>>>>> index 0000000..4f1c713
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/scsi/csiostor/csio_defs.h
>
> <SNIP>
>
>>>>>> +static inline int
>>>>>> +csio_list_deleted(struct list_head *list)
>>>>>> +{
>>>>>> + return ((list->next == list) && (list->prev == list));
>>>>>> +}
>>>>>> +
>>>>>> +#define csio_list_next(elem) (((struct list_head *)(elem))->next)
>>>>>> +#define csio_list_prev(elem) (((struct list_head *)(elem))->prev)
>>>>>> +
>>>>>> +#define csio_deq_from_head(head, elem) \
>>>>>> +do { \
>>>>>> + if (!list_empty(head)) { \
>>>>>> + *((struct list_head **)(elem)) = csio_list_next((head)); \
>>>>>> + csio_list_next((head)) = \
>>>>>> + csio_list_next(csio_list_next((head))); \
>>>>>> + csio_list_prev(csio_list_next((head))) = (head); \
>>>>>> + INIT_LIST_HEAD(*((struct list_head **)(elem))); \
>>>>>> + } else \
>>>>>> + *((struct list_head **)(elem)) = (struct list_head *)NULL;\
>>>>>> +} while (0)
>>>>>> +
>>>>>
>>>>> This code is confusing as hell.. Why can't you just use normal list.h
>>>>> macros for this..?
>>>>
>>>> I have not found an equivalent function in list.h that does the above
>>>> and the following macro. Could you please point me to it? I have seen a
>>>> couple of other drivers define their own macros to achieve what this
>>>> macro does, hence I assumed there isnt a list.h macro that does this.
>>>>
>>>
>>> AFAICT all that csio_deq_from_head code is supposed to do is pull an
>>> item off a list, right..? Why not just:
>>>
>>> while (!list_empty(list)) {
>>> elem = list_first_entry(list, struct elem_type,
>>> elm_list);
>>> list_del_init(&elem->elm_list);
>>>
>>> <do work>
>>> <free *elem memory>
>>> }
>>>
>>
>> I will try to come up with a simpler static inline version of the macro.
>> Would that work?
>
> No. The point is that the above code is a disaster, and AFAICT there is
> no reason why any of it is necessary to begin with at all.
>
> Why can't csio_deq_from_head() just become list_first_entry() +
> list_del_init() to do the exact same thing without all of the extra
> overhead of list_head pointer de-reference + assignments..?
>
> --nab
>
Yes, that's what I was trying to say. csio_deq_from_head() will become
a static function comprising list_first_entry + list_del_init(), with
some checks perhaps.
Thanks,
Naresh.
^ permalink raw reply
* Re: [PATCH 5/8] csiostor: Chelsio FCoE offload driver submission (sources part 5).
From: Nicholas A. Bellinger @ 2012-08-25 18:43 UTC (permalink / raw)
To: Naresh Kumar Inna
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <50391B17.2040505@chelsio.com>
On Sun, 2012-08-26 at 00:06 +0530, Naresh Kumar Inna wrote:
> On 8/25/2012 2:26 AM, Nicholas A. Bellinger wrote:
> > On Fri, 2012-08-24 at 23:10 +0530, Naresh Kumar Inna wrote:
> >> On 8/24/2012 1:18 AM, Nicholas A. Bellinger wrote:
> >>> On Fri, 2012-08-24 at 03:57 +0530, Naresh Kumar Inna wrote:
> >>>> This patch contains code to implement the interrupt handling and the fast
> >>>> path I/O functionality. The interrupt handling includes allocation of
> >>>> MSIX vectors, registering and implemeting the interrupt service routines.
> >>>> The fast path I/O functionality includes posting the I/O request to firmware
> >>>> via Work Requests, tracking/completing them, and handling task management
> >>>> requests. SCSI midlayer host template implementation is also covered by
> >>>> this patch.
> >>>>
> >>>> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
> >>>> ---
> >>>
> >>> Hi Naresh,
> >>>
> >>> My review comments are inline below..
> >>
> >> Hi Nicholas,
> >>
> >> Thanks for taking the time to review the driver. Please find my replies
> >> inline.
> >>
> >> Regards,
> >> Naresh.
> >>
> >>>
> >>>> drivers/scsi/csiostor/csio_isr.c | 631 ++++++++++
> >>>> drivers/scsi/csiostor/csio_scsi.c | 2498 +++++++++++++++++++++++++++++++++++++
> >>>> 2 files changed, 3129 insertions(+), 0 deletions(-)
> >>>> create mode 100644 drivers/scsi/csiostor/csio_isr.c
> >>>> create mode 100644 drivers/scsi/csiostor/csio_scsi.c
> >>>>
<SNIP>
> >>>
> >>>> +/*
> >>>> + * csio_scsi_init_data_wr - Initialize the READ/WRITE SCSI WR.
> >>>> + * @req: IO req structure.
> >>>> + * @oper: read/write
> >>>> + * @wrp: DMA location to place the payload.
> >>>> + * @size: Size of WR (including FW WR + immed data + rsp SG entry + data SGL
> >>>> + * @wrop: _READ_/_WRITE_
> >>>> + *
> >>>> + * Wrapper for populating fw_scsi_read_wr/fw_scsi_write_wr.
> >>>> + */
> >>>> +#define csio_scsi_init_data_wr(req, oper, wrp, size, wrop) \
> >>>> +do { \
> >>>> + struct csio_hw *_hw = (req)->lnode->hwp; \
> >>>> + struct csio_rnode *_rn = (req)->rnode; \
> >>>> + struct fw_scsi_##oper##_wr *__wr = (struct fw_scsi_##oper##_wr *)(wrp);\
> >>>> + struct ulptx_sgl *_sgl; \
> >>>> + struct csio_dma_buf *_dma_buf; \
> >>>> + uint8_t _imm = csio_hw_to_scsim(_hw)->proto_cmd_len; \
> >>>> + struct scsi_cmnd *scmnd = csio_scsi_cmnd((req)); \
> >>>> + \
> >>>> + __wr->op_immdlen = cpu_to_be32(FW_WR_OP(FW_SCSI##wrop##WR) | \
> >>>> + FW_SCSI##wrop##WR_IMMDLEN(_imm)); \
> >>>> + __wr->flowid_len16 = cpu_to_be32(FW_WR_FLOWID(_rn->flowid) | \
> >>>> + FW_WR_LEN16( \
> >>>> + CSIO_ROUNDUP((size), 16))); \
> >>>> + __wr->cookie = (uintptr_t) (req); \
> >>>> + __wr->iqid = (uint16_t)cpu_to_be16(csio_q_physiqid(_hw, \
> >>>> + (req)->iq_idx));\
> >>>> + __wr->tmo_val = (uint8_t)((req)->tmo); \
> >>>> + __wr->use_xfer_cnt = 1; \
> >>>> + __wr->xfer_cnt = cpu_to_be32(scsi_bufflen(scmnd)); \
> >>>> + __wr->ini_xfer_cnt = cpu_to_be32(scsi_bufflen(scmnd)); \
> >>>> + /* Get RSP DMA buffer */ \
> >>>> + _dma_buf = &(req)->dma_buf; \
> >>>> + \
> >>>> + /* Prepare RSP SGL */ \
> >>>> + __wr->rsp_dmalen = cpu_to_be32(_dma_buf->len); \
> >>>> + __wr->rsp_dmaaddr = cpu_to_be64(_dma_buf->paddr); \
> >>>> + \
> >>>> + __wr->r4 = 0; \
> >>>> + \
> >>>> + __wr->u.fcoe.ctl_pri = 0; \
> >>>> + __wr->u.fcoe.cp_en_class = 0; \
> >>>> + __wr->u.fcoe.r3_lo[0] = 0; \
> >>>> + __wr->u.fcoe.r3_lo[1] = 0; \
> >>>> + csio_scsi_fcp_cmnd((req), (void *)((uintptr_t)(wrp) + \
> >>>> + sizeof(struct fw_scsi_##oper##_wr))); \
> >>>> + \
> >>>> + /* Move WR pointer past command and immediate data */ \
> >>>> + _sgl = (struct ulptx_sgl *) ((uintptr_t)(wrp) + \
> >>>> + sizeof(struct fw_scsi_##oper##_wr) + \
> >>>> + ALIGN(_imm, 16)); \
> >>>> + \
> >>>> + /* Fill in the DSGL */ \
> >>>> + csio_scsi_init_ultptx_dsgl(_hw, (req), _sgl); \
> >>>> + \
> >>>> +} while (0)
> >>>> +
> >>>
> >>> This one has four uses of CPP keys. Just turn those into macros, and
> >>> leave the rest of the code in a static function.
> >>>
> >>
> >> So what you are suggesting is to have all the lines of the macro
> >> csio_scsi_init_data_wr() added into a static function, but for the ones
> >> with the 4 keys. csio_scsi_init_data_wr() will then invoke this new
> >> function. Is that correct?
> >>
> >
> > Not sure how the above should actually look without actually doing it,
> > but IMHO the usage of macro just obfuscates what is going on..
> >
> > If it's only used a few times, just inline the code into seperate static
> > functions. If it's used more than a few times, then use a single static
> > funciton with macro accessors for the assignment of the various '__wr'
> > structure members.
> >
> > The larger problem with all of these macros is that you can't tell what
> > is a macro and what is a function.
> >
> > If you need to use a CPP macro, please make sure to capitalize the name
> > of the macro in order to tell the difference between the two.
> >
>
> OK, I will see what I can do to convert this macro into a function.
>
Also please change all of the remaining macro names to be capitalized so
someone reading the code knows the difference between function or macro.
^ permalink raw reply
* Re: [PATCH 6/8] csiostor: Chelsio FCoE offload driver submission (headers part 1).
From: Nicholas A. Bellinger @ 2012-08-25 18:40 UTC (permalink / raw)
To: Naresh Kumar Inna
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <503914C4.4050600@chelsio.com>
On Sat, 2012-08-25 at 23:39 +0530, Naresh Kumar Inna wrote:
> On 8/25/2012 2:47 AM, Nicholas A. Bellinger wrote:
> > On Sat, 2012-08-25 at 00:06 +0530, Naresh Kumar Inna wrote:
> >> On 8/24/2012 1:28 AM, Nicholas A. Bellinger wrote:
> >>> On Fri, 2012-08-24 at 03:57 +0530, Naresh Kumar Inna wrote:
> >>>> This patch contains the first set of the header files for csiostor driver.
> >>>>
> >>>> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
> >>>> ---
> >>>> drivers/scsi/csiostor/csio_defs.h | 143 ++++++
> >>>> drivers/scsi/csiostor/csio_fcoe_proto.h | 843 +++++++++++++++++++++++++++++++
> >>>> drivers/scsi/csiostor/csio_hw.h | 668 ++++++++++++++++++++++++
> >>>> drivers/scsi/csiostor/csio_init.h | 158 ++++++
> >>>> 4 files changed, 1812 insertions(+), 0 deletions(-)
> >>>> create mode 100644 drivers/scsi/csiostor/csio_defs.h
> >>>> create mode 100644 drivers/scsi/csiostor/csio_fcoe_proto.h
> >>>> create mode 100644 drivers/scsi/csiostor/csio_hw.h
> >>>> create mode 100644 drivers/scsi/csiostor/csio_init.h
> >>>>
> >>>
> >>> Hi Naresh,
> >>>
> >>> Just commenting on csio_defs.h bits here... As Robert mentioned, you'll
> >>> need to convert the driver to use (or add to) upstream protocol
> >>> definitions and drop the csio_fcoe_proto.h bits..
> >>>
> >>
> >> Hi Nicholas,
> >>
> >> I would like take up the discussion of the protocol header file in that
> >> email thread. Please find the rest of my replies inline.
> >>
> >> Thanks for reviewing,
> >> Naresh.
> >>
> >>>> diff --git a/drivers/scsi/csiostor/csio_defs.h b/drivers/scsi/csiostor/csio_defs.h
> >>>> new file mode 100644
> >>>> index 0000000..4f1c713
> >>>> --- /dev/null
> >>>> +++ b/drivers/scsi/csiostor/csio_defs.h
<SNIP>
> >>>> +static inline int
> >>>> +csio_list_deleted(struct list_head *list)
> >>>> +{
> >>>> + return ((list->next == list) && (list->prev == list));
> >>>> +}
> >>>> +
> >>>> +#define csio_list_next(elem) (((struct list_head *)(elem))->next)
> >>>> +#define csio_list_prev(elem) (((struct list_head *)(elem))->prev)
> >>>> +
> >>>> +#define csio_deq_from_head(head, elem) \
> >>>> +do { \
> >>>> + if (!list_empty(head)) { \
> >>>> + *((struct list_head **)(elem)) = csio_list_next((head)); \
> >>>> + csio_list_next((head)) = \
> >>>> + csio_list_next(csio_list_next((head))); \
> >>>> + csio_list_prev(csio_list_next((head))) = (head); \
> >>>> + INIT_LIST_HEAD(*((struct list_head **)(elem))); \
> >>>> + } else \
> >>>> + *((struct list_head **)(elem)) = (struct list_head *)NULL;\
> >>>> +} while (0)
> >>>> +
> >>>
> >>> This code is confusing as hell.. Why can't you just use normal list.h
> >>> macros for this..?
> >>
> >> I have not found an equivalent function in list.h that does the above
> >> and the following macro. Could you please point me to it? I have seen a
> >> couple of other drivers define their own macros to achieve what this
> >> macro does, hence I assumed there isnt a list.h macro that does this.
> >>
> >
> > AFAICT all that csio_deq_from_head code is supposed to do is pull an
> > item off a list, right..? Why not just:
> >
> > while (!list_empty(list)) {
> > elem = list_first_entry(list, struct elem_type,
> > elm_list);
> > list_del_init(&elem->elm_list);
> >
> > <do work>
> > <free *elem memory>
> > }
> >
>
> I will try to come up with a simpler static inline version of the macro.
> Would that work?
No. The point is that the above code is a disaster, and AFAICT there is
no reason why any of it is necessary to begin with at all.
Why can't csio_deq_from_head() just become list_first_entry() +
list_del_init() to do the exact same thing without all of the extra
overhead of list_head pointer de-reference + assignments..?
--nab
^ permalink raw reply
* Re: [PATCH 5/8] csiostor: Chelsio FCoE offload driver submission (sources part 5).
From: Naresh Kumar Inna @ 2012-08-25 18:36 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <1345841784.28432.44.camel@haakon2.linux-iscsi.org>
On 8/25/2012 2:26 AM, Nicholas A. Bellinger wrote:
> On Fri, 2012-08-24 at 23:10 +0530, Naresh Kumar Inna wrote:
>> On 8/24/2012 1:18 AM, Nicholas A. Bellinger wrote:
>>> On Fri, 2012-08-24 at 03:57 +0530, Naresh Kumar Inna wrote:
>>>> This patch contains code to implement the interrupt handling and the fast
>>>> path I/O functionality. The interrupt handling includes allocation of
>>>> MSIX vectors, registering and implemeting the interrupt service routines.
>>>> The fast path I/O functionality includes posting the I/O request to firmware
>>>> via Work Requests, tracking/completing them, and handling task management
>>>> requests. SCSI midlayer host template implementation is also covered by
>>>> this patch.
>>>>
>>>> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
>>>> ---
>>>
>>> Hi Naresh,
>>>
>>> My review comments are inline below..
>>
>> Hi Nicholas,
>>
>> Thanks for taking the time to review the driver. Please find my replies
>> inline.
>>
>> Regards,
>> Naresh.
>>
>>>
>>>> drivers/scsi/csiostor/csio_isr.c | 631 ++++++++++
>>>> drivers/scsi/csiostor/csio_scsi.c | 2498 +++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 3129 insertions(+), 0 deletions(-)
>>>> create mode 100644 drivers/scsi/csiostor/csio_isr.c
>>>> create mode 100644 drivers/scsi/csiostor/csio_scsi.c
>>>>
>
> <SNIP>
>
>>>
>>>> diff --git a/drivers/scsi/csiostor/csio_scsi.c b/drivers/scsi/csiostor/csio_scsi.c
>>>> new file mode 100644
>>>> index 0000000..0f87b00
>>>> --- /dev/null
>>>> +++ b/drivers/scsi/csiostor/csio_scsi.c
>>>> +
>>>> +#define CSIO_SCSI_CMD_WR_SZ(_imm) \
>>>> + (sizeof(struct fw_scsi_cmd_wr) + /* WR size */ \
>>>> + ALIGN((_imm), 16)) /* Immed data */
>>>> +
>>>> +#define CSIO_SCSI_CMD_WR_SZ_16(_imm) \
>>>> + (ALIGN(CSIO_SCSI_CMD_WR_SZ((_imm)), 16))
>>>> +
>>>> +/*
>>>> + * csio_scsi_cmd - Create a SCSI CMD WR.
>>>> + * @req: IO req structure.
>>>> + *
>>>> + * Gets a WR slot in the ingress queue and initializes it with SCSI CMD WR.
>>>> + *
>>>> + */
>>>> +static inline void
>>>> +csio_scsi_cmd(struct csio_ioreq *req)
>>>> +{
>>>> + struct csio_wr_pair wrp;
>>>> + struct csio_hw *hw = req->lnode->hwp;
>>>> + struct csio_scsim *scsim = csio_hw_to_scsim(hw);
>>>> + uint32_t size = CSIO_SCSI_CMD_WR_SZ_16(scsim->proto_cmd_len);
>>>> +
>>>> + req->drv_status = csio_wr_get(hw, req->eq_idx, size, &wrp);
>>>> + if (unlikely(req->drv_status != CSIO_SUCCESS))
>>>> + return;
>>>> +
>>>> + if (wrp.size1 >= size) {
>>>> + /* Initialize WR in one shot */
>>>> + csio_scsi_init_cmd_wr(req, wrp.addr1, size);
>>>> + } else {
>>>> + uint8_t tmpwr[512];
>>>
>>> Mmmm, putting this large of a buffer on the local stack is probably not
>>> a good idea.
>>>
>>> This should become an allocation.. If it's a hot path then you'll
>>> probably want to set this up before-hand.
>>>
>>
>> The else switch above is entered only when we near the end of the DMA
>> ring. This is not a frequent occurrence. If it is a strict no-no to have
>> so many on-stack bytes, I have to think of a way re-work it to use
>> pre-allocated memory. Please let me know.
>>
>
> Considering it's used a number of times, it would be better to just
> figure out a sensible manner to pre-allocate this, especially if it's
> only a single occurrence to an individual ring held under a lock.
>
I will see what I can do - I can probably add these bytes into every
queues metadata structure.
>>>> + /*
>>>> + * Make a temporary copy of the WR and write back
>>>> + * the copy into the WR pair.
>>>> + */
>>>> + csio_scsi_init_cmd_wr(req, (void *)tmpwr, size);
>>>> + memcpy(wrp.addr1, tmpwr, wrp.size1);
>>>> + memcpy(wrp.addr2, tmpwr + wrp.size1, size - wrp.size1);
>>>> + }
>>>> +}
>>>> +
>>>> +/*
>>>> + * The following is fast path code. Therefore it is inlined with multi-line
>>>> + * macros using name substitution, thus avoiding if-else switches for
>>>> + * operation (read/write), as well as serving the purpose of code re-use.
>>>> + */
>>>> +/*
>>>> + * csio_scsi_init_ulptx_dsgl - Fill in a ULP_TX_SC_DSGL
>>>> + * @hw: HW module
>>>> + * @req: IO request
>>>> + * @sgl: ULP TX SGL pointer.
>>>> + *
>>>> + */
>>>> +#define csio_scsi_init_ultptx_dsgl(hw, req, sgl) \
>>>> +do { \
>>>> + struct ulptx_sge_pair *_sge_pair = NULL; \
>>>> + struct scatterlist *_sgel; \
>>>> + uint32_t _i = 0; \
>>>> + uint32_t _xfer_len; \
>>>> + struct list_head *_tmp; \
>>>> + struct csio_dma_buf *_dma_buf; \
>>>> + struct scsi_cmnd *scmnd = csio_scsi_cmnd((req)); \
>>>> + \
>>>> + (sgl)->cmd_nsge = htonl(ULPTX_CMD(ULP_TX_SC_DSGL) | ULPTX_MORE | \
>>>> + ULPTX_NSGE((req)->nsge)); \
>>>> + /* Now add the data SGLs */ \
>>>> + if (likely(!(req)->dcopy)) { \
>>>> + scsi_for_each_sg(scmnd, _sgel, (req)->nsge, _i) { \
>>>> + if (_i == 0) { \
>>>> + (sgl)->addr0 = cpu_to_be64( \
>>>> + sg_dma_address(_sgel)); \
>>>> + (sgl)->len0 = cpu_to_be32( \
>>>> + sg_dma_len(_sgel)); \
>>>> + _sge_pair = \
>>>> + (struct ulptx_sge_pair *)((sgl) + 1); \
>>>> + continue; \
>>>> + } \
>>>> + if ((_i - 1) & 0x1) { \
>>>> + _sge_pair->addr[1] = cpu_to_be64( \
>>>> + sg_dma_address(_sgel)); \
>>>> + _sge_pair->len[1] = cpu_to_be32( \
>>>> + sg_dma_len(_sgel)); \
>>>> + _sge_pair++; \
>>>> + } else { \
>>>> + _sge_pair->addr[0] = cpu_to_be64( \
>>>> + sg_dma_address(_sgel)); \
>>>> + _sge_pair->len[0] = cpu_to_be32( \
>>>> + sg_dma_len(_sgel)); \
>>>> + } \
>>>> + } \
>>>> + } else { \
>>>> + /* Program sg elements with driver's DDP buffer */ \
>>>> + _xfer_len = scsi_bufflen(scmnd); \
>>>> + list_for_each(_tmp, &(req)->gen_list) { \
>>>> + _dma_buf = (struct csio_dma_buf *)_tmp; \
>>>> + if (_i == 0) { \
>>>> + (sgl)->addr0 = cpu_to_be64(_dma_buf->paddr); \
>>>> + (sgl)->len0 = cpu_to_be32( \
>>>> + min(_xfer_len, _dma_buf->len)); \
>>>> + _sge_pair = \
>>>> + (struct ulptx_sge_pair *)((sgl) + 1); \
>>>> + } \
>>>> + else if ((_i - 1) & 0x1) { \
>>>> + _sge_pair->addr[1] = cpu_to_be64( \
>>>> + _dma_buf->paddr); \
>>>> + _sge_pair->len[1] = cpu_to_be32( \
>>>> + min(_xfer_len, _dma_buf->len)); \
>>>> + _sge_pair++; \
>>>> + } else { \
>>>> + _sge_pair->addr[0] = cpu_to_be64( \
>>>> + _dma_buf->paddr); \
>>>> + _sge_pair->len[0] = cpu_to_be32( \
>>>> + min(_xfer_len, _dma_buf->len)); \
>>>> + } \
>>>> + _xfer_len -= min(_xfer_len, _dma_buf->len); \
>>>> + _i++; \
>>>> + } \
>>>> + } \
>>>> +} while (0)
>>>> +
>>>
>>> I don't see any reason why this can't just be a static function..? Why
>>> is the macro usage necessary here..?
>>
>> OK, I will make this static-inline.
>>
>>>
>>>> +/*
>>>> + * csio_scsi_init_data_wr - Initialize the READ/WRITE SCSI WR.
>>>> + * @req: IO req structure.
>>>> + * @oper: read/write
>>>> + * @wrp: DMA location to place the payload.
>>>> + * @size: Size of WR (including FW WR + immed data + rsp SG entry + data SGL
>>>> + * @wrop: _READ_/_WRITE_
>>>> + *
>>>> + * Wrapper for populating fw_scsi_read_wr/fw_scsi_write_wr.
>>>> + */
>>>> +#define csio_scsi_init_data_wr(req, oper, wrp, size, wrop) \
>>>> +do { \
>>>> + struct csio_hw *_hw = (req)->lnode->hwp; \
>>>> + struct csio_rnode *_rn = (req)->rnode; \
>>>> + struct fw_scsi_##oper##_wr *__wr = (struct fw_scsi_##oper##_wr *)(wrp);\
>>>> + struct ulptx_sgl *_sgl; \
>>>> + struct csio_dma_buf *_dma_buf; \
>>>> + uint8_t _imm = csio_hw_to_scsim(_hw)->proto_cmd_len; \
>>>> + struct scsi_cmnd *scmnd = csio_scsi_cmnd((req)); \
>>>> + \
>>>> + __wr->op_immdlen = cpu_to_be32(FW_WR_OP(FW_SCSI##wrop##WR) | \
>>>> + FW_SCSI##wrop##WR_IMMDLEN(_imm)); \
>>>> + __wr->flowid_len16 = cpu_to_be32(FW_WR_FLOWID(_rn->flowid) | \
>>>> + FW_WR_LEN16( \
>>>> + CSIO_ROUNDUP((size), 16))); \
>>>> + __wr->cookie = (uintptr_t) (req); \
>>>> + __wr->iqid = (uint16_t)cpu_to_be16(csio_q_physiqid(_hw, \
>>>> + (req)->iq_idx));\
>>>> + __wr->tmo_val = (uint8_t)((req)->tmo); \
>>>> + __wr->use_xfer_cnt = 1; \
>>>> + __wr->xfer_cnt = cpu_to_be32(scsi_bufflen(scmnd)); \
>>>> + __wr->ini_xfer_cnt = cpu_to_be32(scsi_bufflen(scmnd)); \
>>>> + /* Get RSP DMA buffer */ \
>>>> + _dma_buf = &(req)->dma_buf; \
>>>> + \
>>>> + /* Prepare RSP SGL */ \
>>>> + __wr->rsp_dmalen = cpu_to_be32(_dma_buf->len); \
>>>> + __wr->rsp_dmaaddr = cpu_to_be64(_dma_buf->paddr); \
>>>> + \
>>>> + __wr->r4 = 0; \
>>>> + \
>>>> + __wr->u.fcoe.ctl_pri = 0; \
>>>> + __wr->u.fcoe.cp_en_class = 0; \
>>>> + __wr->u.fcoe.r3_lo[0] = 0; \
>>>> + __wr->u.fcoe.r3_lo[1] = 0; \
>>>> + csio_scsi_fcp_cmnd((req), (void *)((uintptr_t)(wrp) + \
>>>> + sizeof(struct fw_scsi_##oper##_wr))); \
>>>> + \
>>>> + /* Move WR pointer past command and immediate data */ \
>>>> + _sgl = (struct ulptx_sgl *) ((uintptr_t)(wrp) + \
>>>> + sizeof(struct fw_scsi_##oper##_wr) + \
>>>> + ALIGN(_imm, 16)); \
>>>> + \
>>>> + /* Fill in the DSGL */ \
>>>> + csio_scsi_init_ultptx_dsgl(_hw, (req), _sgl); \
>>>> + \
>>>> +} while (0)
>>>> +
>>>
>>> This one has four uses of CPP keys. Just turn those into macros, and
>>> leave the rest of the code in a static function.
>>>
>>
>> So what you are suggesting is to have all the lines of the macro
>> csio_scsi_init_data_wr() added into a static function, but for the ones
>> with the 4 keys. csio_scsi_init_data_wr() will then invoke this new
>> function. Is that correct?
>>
>
> Not sure how the above should actually look without actually doing it,
> but IMHO the usage of macro just obfuscates what is going on..
>
> If it's only used a few times, just inline the code into seperate static
> functions. If it's used more than a few times, then use a single static
> funciton with macro accessors for the assignment of the various '__wr'
> structure members.
>
> The larger problem with all of these macros is that you can't tell what
> is a macro and what is a function.
>
> If you need to use a CPP macro, please make sure to capitalize the name
> of the macro in order to tell the difference between the two.
>
OK, I will see what I can do to convert this macro into a function.
>>>> +/* Calculate WR size needed for fw_scsi_read_wr/fw_scsi_write_wr */
>>>> +#define csio_scsi_data_wrsz(req, oper, sz, imm) \
>>>> +do { \
>>>> + (sz) = sizeof(struct fw_scsi_##oper##_wr) + /* WR size */ \
>>>> + ALIGN((imm), 16) + /* Immed data */ \
>>>> + sizeof(struct ulptx_sgl); /* ulptx_sgl */ \
>>>> + \
>>>> + if (unlikely((req)->nsge > 1)) \
>>>> + (sz) += (sizeof(struct ulptx_sge_pair) * \
>>>> + (ALIGN(((req)->nsge - 1), 2) / 2)); \
>>>> + /* Data SGE */ \
>>>> +} while (0)
>>>> +
>>>> +/*
>>>> + * csio_scsi_data - Create a SCSI WRITE/READ WR.
>>>> + * @req: IO req structure.
>>>> + * @oper: read/write
>>>> + * @wrop: _READ_/_WRITE_ (string subsitutions to use with the FW bit field
>>>> + * macros).
>>>> + *
>>>> + * Gets a WR slot in the ingress queue and initializes it with
>>>> + * SCSI CMD READ/WRITE WR.
>>>> + *
>>>> + */
>>>> +#define csio_scsi_data(req, oper, wrop) \
>>>> +do { \
>>>> + struct csio_wr_pair _wrp; \
>>>> + uint32_t _size; \
>>>> + struct csio_hw *_hw = (req)->lnode->hwp; \
>>>> + struct csio_scsim *_scsim = csio_hw_to_scsim(_hw); \
>>>> + \
>>>> + csio_scsi_data_wrsz((req), oper, _size, _scsim->proto_cmd_len); \
>>>> + _size = ALIGN(_size, 16); \
>>>> + \
>>>> + (req)->drv_status = csio_wr_get(_hw, (req)->eq_idx, _size, &_wrp); \
>>>> + if (likely((req)->drv_status == CSIO_SUCCESS)) { \
>>>> + if (likely(_wrp.size1 >= _size)) { \
>>>> + /* Initialize WR in one shot */ \
>>>> + csio_scsi_init_data_wr((req), oper, _wrp.addr1, \
>>>> + _size, wrop); \
>>>> + } else { \
>>>> + uint8_t tmpwr[512]; \
>>>> + /* \
>>>> + * Make a temporary copy of the WR and write back \
>>>> + * the copy into the WR pair. \
>>>> + */ \
>>>> + csio_scsi_init_data_wr((req), oper, (void *)tmpwr, \
>>>> + _size, wrop); \
>>>> + memcpy(_wrp.addr1, tmpwr, _wrp.size1); \
>>>> + memcpy(_wrp.addr2, tmpwr + _wrp.size1, \
>>>> + _size - _wrp.size1); \
>>>> + } \
>>>> + } \
>>>> +} while (0)
>>>> +
>>>
>>> Ditto on this one, along with the tmpwr[512] stack usage..
>>>
>>>> +static inline void
>>>> +csio_scsi_abrt_cls(struct csio_ioreq *req, bool abort)
>>>> +{
>>>> + struct csio_wr_pair wrp;
>>>> + struct csio_hw *hw = req->lnode->hwp;
>>>> + uint32_t size = ALIGN(sizeof(struct fw_scsi_abrt_cls_wr), 16);
>>>> +
>>>> + req->drv_status = csio_wr_get(hw, req->eq_idx, size, &wrp);
>>>> + if (req->drv_status != CSIO_SUCCESS)
>>>> + return;
>>>> +
>>>> + if (wrp.size1 >= size) {
>>>> + /* Initialize WR in one shot */
>>>> + csio_scsi_init_abrt_cls_wr(req, wrp.addr1, size, abort);
>>>> + } else {
>>>> + uint8_t tmpwr[512];
>>>
>>> Ditto here on local scope stack usage..
>>>
>>>> + /*
>>>> + * Make a temporary copy of the WR and write back
>>>> + * the copy into the WR pair.
>>>> + */
>>>> + csio_scsi_init_abrt_cls_wr(req, (void *)tmpwr, size, abort);
>>>> + memcpy(wrp.addr1, tmpwr, wrp.size1);
>>>> + memcpy(wrp.addr2, tmpwr + wrp.size1, size - wrp.size1);
>>>> + }
>>>> +}
>>>> +
>>>> +/*****************************************************************************/
>>>> +/* START: SCSI SM */
>>>> +/*****************************************************************************/
>>>> +static void
>>>> +csio_scsis_uninit(struct csio_ioreq *req, enum csio_scsi_ev evt)
>>>> +{
>>>> + struct csio_hw *hw = req->lnode->hwp;
>>>> + struct csio_scsim *scsim = csio_hw_to_scsim(hw);
>>>> +
>>>> + switch (evt) {
>>>> +
>>>> + case CSIO_SCSIE_START_IO:
>>>
>>> Extra space between start of first switch case
>>
>> OK, I will remove it. Is there any tool that catches such deviations?
>> checkpath.pl didnt report it.
>>
>
> Sorry, can't help you there.. ;)
>
>>>
>>>> +
>>>> +/**
>>>> + * csio_queuecommand_lck - Entry point to kickstart an I/O request.
>>>> + * @cmnd: The I/O request from ML.
>>>> + * @done: The ML callback routine.
>>>> + *
>>>> + * This routine does the following:
>>>> + * - Checks for HW and Rnode module readiness.
>>>> + * - Gets a free ioreq structure (which is already initialized
>>>> + * to uninit during its allocation).
>>>> + * - Maps SG elements.
>>>> + * - Initializes ioreq members.
>>>> + * - Kicks off the SCSI state machine for this IO.
>>>> + * - Returns busy status on error.
>>>> + */
>>>> +static int
>>>> +csio_queuecommand_lck(struct scsi_cmnd *cmnd, void (*done)(struct scsi_cmnd *))
>>>> +{
>>>> + struct csio_lnode *ln = shost_priv(cmnd->device->host);
>>>> + struct csio_hw *hw = csio_lnode_to_hw(ln);
>>>> + struct csio_scsim *scsim = csio_hw_to_scsim(hw);
>>>> + struct csio_rnode *rn = (struct csio_rnode *)(cmnd->device->hostdata);
>>>> + struct csio_ioreq *ioreq = NULL;
>>>> + unsigned long flags;
>>>> + int nsge = 0;
>>>> + int rv = SCSI_MLQUEUE_HOST_BUSY, nr;
>>>> + csio_retval_t retval;
>>>> + int cpu;
>>>> + struct csio_scsi_qset *sqset;
>>>> + struct fc_rport *rport = starget_to_rport(scsi_target(cmnd->device));
>>>> +
>>>> + if (!blk_rq_cpu_valid(cmnd->request))
>>>> + cpu = smp_processor_id();
>>>> + else
>>>> + cpu = cmnd->request->cpu;
>>>> +
>>>> + sqset = &hw->sqset[ln->portid][cpu];
>>>> +
>>>> + nr = fc_remote_port_chkready(rport);
>>>> + if (nr) {
>>>> + cmnd->result = nr;
>>>> + csio_inc_stats(scsim, n_rn_nr_error);
>>>> + goto err_done;
>>>> + }
>>>> +
>>>> + if (unlikely(!csio_is_hw_ready(hw))) {
>>>> + cmnd->result = (DID_REQUEUE << 16);
>>>> + csio_inc_stats(scsim, n_hw_nr_error);
>>>> + goto err_done;
>>>> + }
>>>> +
>>>> + /* Get req->nsge, if there are SG elements to be mapped */
>>>> + nsge = scsi_dma_map(cmnd);
>>>> + if (unlikely(nsge < 0)) {
>>>> + csio_inc_stats(scsim, n_dmamap_error);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + /* Do we support so many mappings? */
>>>> + if (unlikely(nsge > scsim->max_sge)) {
>>>> + csio_warn(hw,
>>>> + "More SGEs than can be supported."
>>>> + " SGEs: %d, Max SGEs: %d\n", nsge, scsim->max_sge);
>>>> + csio_inc_stats(scsim, n_unsupp_sge_error);
>>>> + goto err_dma_unmap;
>>>> + }
>>>> +
>>>> + /* Get a free ioreq structure - SM is already set to uninit */
>>>> + ioreq = csio_get_scsi_ioreq_lock(hw, scsim);
>>>> + if (!ioreq) {
>>>> + csio_err(hw, "Out of I/O request elements. Active #:%d\n",
>>>> + scsim->stats.n_active);
>>>> + csio_inc_stats(scsim, n_no_req_error);
>>>> + goto err_dma_unmap;
>>>> + }
>>>> +
>>>> + ioreq->nsge = nsge;
>>>> + ioreq->lnode = ln;
>>>> + ioreq->rnode = rn;
>>>> + ioreq->iq_idx = sqset->iq_idx;
>>>> + ioreq->eq_idx = sqset->eq_idx;
>>>> + ioreq->wr_status = 0;
>>>> + ioreq->drv_status = CSIO_SUCCESS;
>>>> + csio_scsi_cmnd(ioreq) = (void *)cmnd;
>>>> + ioreq->tmo = 0;
>>>> +
>>>> + switch (cmnd->sc_data_direction) {
>>>> + case DMA_BIDIRECTIONAL:
>>>> + ioreq->datadir = CSIO_IOREQF_DMA_BIDI;
>>>> + csio_inc_stats(ln, n_control_requests);
>>>> + break;
>>>> + case DMA_TO_DEVICE:
>>>> + ioreq->datadir = CSIO_IOREQF_DMA_WRITE;
>>>> + csio_inc_stats(ln, n_output_requests);
>>>> + ln->stats.n_output_bytes += scsi_bufflen(cmnd);
>>>> + break;
>>>> + case DMA_FROM_DEVICE:
>>>> + ioreq->datadir = CSIO_IOREQF_DMA_READ;
>>>> + csio_inc_stats(ln, n_input_requests);
>>>> + ln->stats.n_input_bytes += scsi_bufflen(cmnd);
>>>> + break;
>>>> + case DMA_NONE:
>>>> + ioreq->datadir = CSIO_IOREQF_DMA_NONE;
>>>> + csio_inc_stats(ln, n_control_requests);
>>>> + break;
>>>> + default:
>>>> + CSIO_DB_ASSERT(0);
>>>> + break;
>>>> + }
>>>> +
>>>> + /* Set cbfn */
>>>> + ioreq->io_cbfn = csio_scsi_cbfn;
>>>> +
>>>> + /* Needed during abort */
>>>> + cmnd->host_scribble = (unsigned char *)ioreq;
>>>> + cmnd->scsi_done = done;
>>>> + cmnd->SCp.Message = 0;
>>>> +
>>>> + /* Kick off SCSI IO SM on the ioreq */
>>>> + spin_lock_irqsave(&hw->lock, flags);
>>>> + retval = csio_scsi_start_io(ioreq);
>>>> + spin_unlock_irqrestore(&hw->lock, flags);
>>>> +
>>>> + if (retval != CSIO_SUCCESS) {
>>>> + csio_err(hw, "ioreq: %p couldnt be started, status:%d\n",
>>>> + ioreq, retval);
>>>> + csio_inc_stats(scsim, n_busy_error);
>>>> + goto err_put_req;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +
>>>> +err_put_req:
>>>> + csio_put_scsi_ioreq_lock(hw, scsim, ioreq);
>>>> +err_dma_unmap:
>>>> + if (nsge > 0)
>>>> + scsi_dma_unmap(cmnd);
>>>> +err:
>>>> + return rv;
>>>> +
>>>> +err_done:
>>>> + done(cmnd);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static DEF_SCSI_QCMD(csio_queuecommand);
>>>> +
>>>
>>> This means that your running with the host_lock held.. I'm not sure if
>>> that is really what you want to do as it really end's up killing
>>> multi-lun small packet performance..
>>>
>>> How about dropping DEF_SCSI_QCMD usage here, and figure out what
>>> actually needs to be protected by the SCSI host_lock within
>>> csio_queuecommand_lck()..?
>>
>> It is on my TODO list for the next version of the driver, after the
>> initial submission. Per the current design, we shouldnt need the
>> host_lock to be held, but I would like to test this change thoroughly
>> before I submit it.
>>
>
> Sure, it's really quite easy to convert and these days the majority of
> high performance LLDs do run in host-lock-less mode.
>
> IIRC the libfc based FCoE initiator driver is doing this too..
>
^ permalink raw reply
* Re: [PATCH 6/8] csiostor: Chelsio FCoE offload driver submission (headers part 1).
From: Naresh Kumar Inna @ 2012-08-25 18:09 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <1345843042.28432.65.camel@haakon2.linux-iscsi.org>
On 8/25/2012 2:47 AM, Nicholas A. Bellinger wrote:
> On Sat, 2012-08-25 at 00:06 +0530, Naresh Kumar Inna wrote:
>> On 8/24/2012 1:28 AM, Nicholas A. Bellinger wrote:
>>> On Fri, 2012-08-24 at 03:57 +0530, Naresh Kumar Inna wrote:
>>>> This patch contains the first set of the header files for csiostor driver.
>>>>
>>>> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
>>>> ---
>>>> drivers/scsi/csiostor/csio_defs.h | 143 ++++++
>>>> drivers/scsi/csiostor/csio_fcoe_proto.h | 843 +++++++++++++++++++++++++++++++
>>>> drivers/scsi/csiostor/csio_hw.h | 668 ++++++++++++++++++++++++
>>>> drivers/scsi/csiostor/csio_init.h | 158 ++++++
>>>> 4 files changed, 1812 insertions(+), 0 deletions(-)
>>>> create mode 100644 drivers/scsi/csiostor/csio_defs.h
>>>> create mode 100644 drivers/scsi/csiostor/csio_fcoe_proto.h
>>>> create mode 100644 drivers/scsi/csiostor/csio_hw.h
>>>> create mode 100644 drivers/scsi/csiostor/csio_init.h
>>>>
>>>
>>> Hi Naresh,
>>>
>>> Just commenting on csio_defs.h bits here... As Robert mentioned, you'll
>>> need to convert the driver to use (or add to) upstream protocol
>>> definitions and drop the csio_fcoe_proto.h bits..
>>>
>>
>> Hi Nicholas,
>>
>> I would like take up the discussion of the protocol header file in that
>> email thread. Please find the rest of my replies inline.
>>
>> Thanks for reviewing,
>> Naresh.
>>
>>>> diff --git a/drivers/scsi/csiostor/csio_defs.h b/drivers/scsi/csiostor/csio_defs.h
>>>> new file mode 100644
>>>> index 0000000..4f1c713
>>>> --- /dev/null
>>>> +++ b/drivers/scsi/csiostor/csio_defs.h
>
> <SNIP>
>
>>>> +#ifndef __CSIO_DEFS_H__
>>>> +#define __CSIO_DEFS_H__
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/timer.h>
>>>> +#include <linux/list.h>
>>>> +#include <linux/bug.h>
>>>> +#include <linux/pci.h>
>>>> +#include <linux/jiffies.h>
>>>> +
>>>> +/* Function returns */
>>>> +enum csio_retval {
>>>> + CSIO_SUCCESS = 0,
>>>> + CSIO_INVAL = 1,
>>>> + CSIO_BUSY = 2,
>>>> + CSIO_NOSUPP = 3,
>>>> + CSIO_TIMEOUT = 4,
>>>> + CSIO_NOMEM = 5,
>>>> + CSIO_NOPERM = 6,
>>>> + CSIO_RETRY = 7,
>>>> + CSIO_EPROTO = 8,
>>>> + CSIO_EIO = 9,
>>>> + CSIO_CANCELLED = 10,
>>>> +};
>>>> +
>>>
>>> Please don't assign macros for errno's, and give them positive values.
>>>
>>
>> Although some of these return values appear to be mapped to errno
>> values, there are others that do not have an errno equivalent (example
>> CSIO_CANCELLED). We may have future needs to have more driver/protocol
>> specific return values as well. What do you suggest?
>>
>
> Convert all functions aside from CSIO_CANCELLED to use normal negative
> return values from include/asm-generic/error[-base].h
>
> For the CSIO_CANCELLED case, propagate this status up to the specific
> caller using another method..
>
>>>> +#define csio_retval_t enum csio_retval
>>>
>>> Please get rid of this csio_retval_t nonsense.
>>
>> I can get rid of the typedef and use enum csio_retval instead.
>>
>
> Using a LLD defined retval where %90 of the items are from errno.h is
> code duplication. Please get rid of this.
>
OK I will switch over to the errno values.
>>>
>>>> +
>>>> +enum {
>>>> + CSIO_FALSE = 0,
>>>> + CSIO_TRUE = 1,
>>>> +};
>>>> +
>>>
>>> Same here, please use normal Boolean macros
>>>
>>>> +#define CSIO_ROUNDUP(__v, __r) (((__v) + (__r) - 1) / (__r))
>>>> +#define CSIO_INVALID_IDX 0xFFFFFFFF
>>>> +#define csio_inc_stats(elem, val) ((elem)->stats.val++)
>>>> +#define csio_dec_stats(elem, val) ((elem)->stats.val--)
>>>
>>> No reason for either of this stats inc+dec macros. Please drop them.
>>
>> I will get rid of them.
>>
>>>
>>>> +#define csio_valid_wwn(__n) ((*__n >> 4) == 0x5 ? CSIO_TRUE : \
>>>> + CSIO_FALSE)
>>>> +#define CSIO_WORD_TO_BYTE 4
>>>> +
>>>> +static inline int
>>>> +csio_list_deleted(struct list_head *list)
>>>> +{
>>>> + return ((list->next == list) && (list->prev == list));
>>>> +}
>>>> +
>>>> +#define csio_list_next(elem) (((struct list_head *)(elem))->next)
>>>> +#define csio_list_prev(elem) (((struct list_head *)(elem))->prev)
>>>> +
>>>> +#define csio_deq_from_head(head, elem) \
>>>> +do { \
>>>> + if (!list_empty(head)) { \
>>>> + *((struct list_head **)(elem)) = csio_list_next((head)); \
>>>> + csio_list_next((head)) = \
>>>> + csio_list_next(csio_list_next((head))); \
>>>> + csio_list_prev(csio_list_next((head))) = (head); \
>>>> + INIT_LIST_HEAD(*((struct list_head **)(elem))); \
>>>> + } else \
>>>> + *((struct list_head **)(elem)) = (struct list_head *)NULL;\
>>>> +} while (0)
>>>> +
>>>
>>> This code is confusing as hell.. Why can't you just use normal list.h
>>> macros for this..?
>>
>> I have not found an equivalent function in list.h that does the above
>> and the following macro. Could you please point me to it? I have seen a
>> couple of other drivers define their own macros to achieve what this
>> macro does, hence I assumed there isnt a list.h macro that does this.
>>
>
> AFAICT all that csio_deq_from_head code is supposed to do is pull an
> item off a list, right..? Why not just:
>
> while (!list_empty(list)) {
> elem = list_first_entry(list, struct elem_type,
> elm_list);
> list_del_init(&elem->elm_list);
>
> <do work>
> <free *elem memory>
> }
>
I will try to come up with a simpler static inline version of the macro.
Would that work?
>>>> +#define csio_deq_from_tail(head, elem) \
>>>> +do { \
>>>> + if (!list_empty(head)) { \
>>>> + *((struct list_head **)(elem)) = csio_list_prev((head)); \
>>>> + csio_list_prev((head)) = \
>>>> + csio_list_prev(csio_list_prev((head))); \
>>>> + csio_list_next(csio_list_prev((head))) = (head); \
>>>> + INIT_LIST_HEAD(*((struct list_head **)(elem))); \
>>>> + } else \
>>>> + *((struct list_head **)(elem)) = (struct list_head *)NULL;\
>>>> +} while (0)
>>>> +
>>>
>>> Same here.. Please don't use macros like this.
>>>
>
> AFIACT csio_deq_from_tail is unused..?
>
> Please remove it..
>
I will remove it.
^ permalink raw reply
* Re: BUG: soft lockup - CPU#6 stuck for 22s! [httpd2-event:15597]
From: Eric Dumazet @ 2012-08-25 17:52 UTC (permalink / raw)
To: Cristian Rodríguez; +Cc: netdev, Yuchung Cheng, Neal Cardwell
In-Reply-To: <1345895225.19483.447.camel@edumazet-glaptop>
On Sat, 2012-08-25 at 13:47 +0200, Eric Dumazet wrote:
> On Sat, 2012-08-25 at 11:14 +0200, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > On Sat, 2012-08-25 at 10:59 +0200, Eric Dumazet wrote:
> > > On Fri, 2012-08-24 at 20:50 -0400, Cristian Rodríguez wrote:
> > > > Hi, the issue I reported with IPV6 few weeks ago seems to be gone, but
> > > > now I am getting the following crash..
> >
> > > Oh, I now see the bug, I'll send a patch asap
> >
> > Please try the following fix.
> >
> > Thanks !
>
> Well, this v2 seems cleaner :
>
> [PATCH v2] tcp: tcp_slow_start() should not decrease snd_cwnd
>
> Cristian Rodríguez reported various lockups in TCP stack,
> introduced by commit 9dc274151a548 (tcp: fix ABC in tcp_slow_start())
>
> We could exit tcp_slow_start() with a zeroed snd_cwnd,
> and next time we enter tcp_slow_start(), we run an infinite loop.
>
> Reported-by: Cristian Rodríguez <crrodriguez@opensuse.org>
> Cc: Yuchung Cheng <ycheng@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> net/ipv4/tcp_cong.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
> index 1432cdb..e656c72 100644
> --- a/net/ipv4/tcp_cong.c
> +++ b/net/ipv4/tcp_cong.c
> @@ -337,7 +337,7 @@ void tcp_slow_start(struct tcp_sock *tp)
> tp->snd_cwnd_cnt -= tp->snd_cwnd;
> delta++;
> }
> - tp->snd_cwnd = min(tp->snd_cwnd + delta, tp->snd_cwnd_clamp);
> + tp->snd_cwnd = clamp(tp->snd_cwnd + delta, tp->snd_cwnd, tp->snd_cwnd_clamp);
> }
> EXPORT_SYMBOL_GPL(tcp_slow_start);
>
>
Hmm...
We probably have a bug in tcp_metrics.c, because snd_cwnd_clamp should
not be zero.
With RCU, it seems following code in tcpm_new() is racy :
tm->tcpm_addr = *addr;
...
tcpm_suck_dst(tm, dst);
Coupled with the code in tcpm_suck_dst(tm, dst)
static void tcpm_suck_dst(struct tcp_metrics_block *tm, struct dst_entry *dst)
{
u32 val;
tm->tcpm_stamp = jiffies;
val = 0;
if (dst_metric_locked(dst, RTAX_RTT))
val |= 1 << TCP_METRIC_RTT;
if (dst_metric_locked(dst, RTAX_RTTVAR))
val |= 1 << TCP_METRIC_RTTVAR;
if (dst_metric_locked(dst, RTAX_SSTHRESH))
val |= 1 << TCP_METRIC_SSTHRESH;
if (dst_metric_locked(dst, RTAX_CWND))
val |= 1 << TCP_METRIC_CWND;
if (dst_metric_locked(dst, RTAX_REORDERING))
val |= 1 << TCP_METRIC_REORDERING;
tm->tcpm_lock = val;
// HERE we set tcpm_lock before the tcpm_vals[]
tm->tcpm_vals[TCP_METRIC_RTT] = dst_metric_raw(dst, RTAX_RTT);
tm->tcpm_vals[TCP_METRIC_RTTVAR] = dst_metric_raw(dst, RTAX_RTTVAR);
tm->tcpm_vals[TCP_METRIC_SSTHRESH] = dst_metric_raw(dst, RTAX_SSTHRESH);
tm->tcpm_vals[TCP_METRIC_CWND] = dst_metric_raw(dst, RTAX_CWND);
tm->tcpm_vals[TCP_METRIC_REORDERING] = dst_metric_raw(dst, RTAX_REORDERING);
tm->tcpm_ts = 0;
tm->tcpm_ts_stamp = 0;
tm->tcpm_fastopen.mss = 0;
tm->tcpm_fastopen.syn_loss = 0;
tm->tcpm_fastopen.cookie.len = 0;
}
^ permalink raw reply
* Re: BUG: soft lockup - CPU#6 stuck for 22s! [httpd2-event:15597]
From: Cristian Rodríguez @ 2012-08-25 17:45 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Yuchung Cheng, Neal Cardwell, Florian Schmitz
In-Reply-To: <1345895225.19483.447.camel@edumazet-glaptop>
El 25/08/12 07:47, Eric Dumazet escribió:
> On Sat, 2012-08-25 at 11:14 +0200, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> On Sat, 2012-08-25 at 10:59 +0200, Eric Dumazet wrote:
>>> On Fri, 2012-08-24 at 20:50 -0400, Cristian Rodríguez wrote:
>>>> Hi, the issue I reported with IPV6 few weeks ago seems to be gone, but
>>>> now I am getting the following crash..
>>
>>> Oh, I now see the bug, I'll send a patch asap
>>
>> Please try the following fix.
>>
>> Thanks !
Eric, thanks for your work, I have deployed the fix in one machine and
will keep an eye for any other anomaly.
As it happends around twice a day, it would take a while to determine if
this issue has gone away.
Keep up great work :-)
^ permalink raw reply
* Re: [PATCH v2 06/10] cgroup: Assign subsystem IDs during compile time
From: Daniel Wagner @ 2012-08-25 17:11 UTC (permalink / raw)
To: Tejun Heo
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
David S. Miller, Andrew Morton, Eric Dumazet, Gao feng,
Glauber Costa, Jamal Hadi Salim, John Fastabend,
Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <20120824233810.GT21325-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On 25.08.2012 01:38, Tejun Heo wrote:
> Hello,
>
> On Fri, Aug 24, 2012 at 04:01:40PM +0200, Daniel Wagner wrote:
>> We are able to safe some space when we assign the subsystem
> ^ ^
> save if we assign both builtin and
> module susbsystem IDs at compile time?
>
>> IDs at compile time. Instead of allocating per cgroup
>> cgroup->subsys[CGROUP_SUBSYS_COUNT] where CGROUP_SUBSYS_COUNT is
>> always 64, we allocate at max 12 (at this point there are 12
>> subsystem).
>
> Please note (in big fat ugly way) that this disallows support for
> modular controller which isn't known at kernel compile time.
yep, will do
>> task_cls_classid() and task_netprioidx() (when built as
>> module) are protected by a jump label and therefore we can
>> simply replace the subsystem index lookup with the enum.
>
> Can we put these in a separate patch? ie. The first patch makes all
> subsys IDs constant and then patches to simplify users.
Wouldn't this break bisection? I merged this step so that all steps in
this series are able to compile and run.
>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>> index 3787872..ada517f 100644
>> --- a/include/linux/cgroup.h
>> +++ b/include/linux/cgroup.h
>> @@ -43,18 +43,27 @@ extern void cgroup_unload_subsys(struct cgroup_subsys *ss);
>>
>> extern const struct file_operations proc_cgroup_operations;
>>
>> -/* Define the enumeration of all builtin cgroup subsystems */
>> -#define SUBSYS(_x) _x ## _subsys_id,
>> +/*
>> + * Define the enumeration of all builtin cgroup subsystems.
>> + * For the builtin subsystems the subsys_id needs to be indentical
>> + * with the index in css->subsys. Therefore, all the builtin
>> + * subsys are listed first and then the modules ids.
>> + */
>> enum cgroup_subsys_id {
>> +#define SUBSYS(_x) _x ## _subsys_id,
>> +
>> +#define IS_SUBSYS_ENABLED(option) IS_BUILTIN(option)
>> #include <linux/cgroup_subsys.h>
>> -};
>> +#undef IS_SUBSYS_ENABLED
>> +
>> +#define IS_SUBSYS_ENABLED(option) IS_MODULE(option)
>> +#include <linux/cgroup_subsys.h>
>> +#undef IS_SUBSYS_ENABLED
>> +
>
> Why do we need to segregate in-kernel and modular ones at all? What's
> wrong with just defining them in one go?
I have done that but the result was a panic. There seems some code which
expects this ordering. Let me dig into this and fix it.
>> diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
>> index 24443d2..43fae13 100644
>> --- a/include/net/cls_cgroup.h
>> +++ b/include/net/cls_cgroup.h
>> @@ -49,22 +49,16 @@ static inline u32 task_cls_classid(struct task_struct *p)
>> extern struct static_key cgroup_cls_enabled;
>> #define clscg_enabled static_key_false(&cgroup_cls_enabled)
>>
>> -extern int net_cls_subsys_id;
>> -
>> static inline u32 task_cls_classid(struct task_struct *p)
>> {
>> - int id;
>> - u32 classid = 0;
>> + u32 classid;
>>
>> if (!clscg_enabled || in_interrupt())
>> return 0;
>>
>> rcu_read_lock();
>> - id = rcu_dereference_index_check(net_cls_subsys_id,
>> - rcu_read_lock_held());
>> - if (id >= 0)
>> - classid = container_of(task_subsys_state(p, id),
>> - struct cgroup_cls_state, css)->classid;
>> + classid = container_of(task_subsys_state(p, net_cls_subsys_id),
>> + struct cgroup_cls_state, css)->classid;
>> rcu_read_unlock();
>>
>> return classid;
>
> Hmm... patch sequence looks odd to me. If you first make all IDs
> constant, you can first remove module specific ones and then later add
> jump labels as separate patches. Wouldn't that be simpler?
As said above, I tried to keep all steps usable so bisection would work.
I think your steps would lead to non working versions of the kernel.
^ permalink raw reply
* Re: [PATCH v2 05/10] cgroup: Remove CGROUP_BUILTIN_SUBSYS_COUNT
From: Daniel Wagner @ 2012-08-25 16:59 UTC (permalink / raw)
To: Tejun Heo
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Li Zefan, David S. Miller
In-Reply-To: <20120824232840.GS21325-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On 25.08.2012 01:28, Tejun Heo wrote:
> On Fri, Aug 24, 2012 at 04:01:39PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>>
>> CGROUP_BUILTIN_SUBSYS_COUNT is used as start or stop point
>> when looping over the subsys array. Since the subsys array is
>> 64 entries long this is a good thing to do. Though we'd like
>> to reduce the array size considerable but we need to get rid
>> of CGROUP_BUILTIN_SUBSYS_COUNT to ease up the review process.
>
> Wouldn't it be better to explicitly state that a following patch would
> reduce the SUBSYS_COUNT and stop putting builtin and module ones into
> different sections?
Sure, can do that. I just to make sure I understand you correctly,
What do you mean with different section? Do you refer to the enum sorting?
^ permalink raw reply
* Re: [PATCH v2 03/10] cgroup: net_cls: Protect access to task_cls_classid() when built as module
From: Daniel Wagner @ 2012-08-25 16:56 UTC (permalink / raw)
To: Tejun Heo
Cc: netdev, cgroups, David S. Miller, Gao feng, Jamal Hadi Salim,
John Fastabend, Li Zefan, Neil Horman
In-Reply-To: <20120824232621.GQ21325@google.com>
On 25.08.2012 01:26, Tejun Heo wrote:
> On Fri, Aug 24, 2012 at 04:01:37PM +0200, Daniel Wagner wrote:
>> @@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void)
>> synchronize_rcu();
>> #endif
>>
>> +#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
>> + static_key_slow_dec(&cgroup_cls_enabled);
>> + rcu_barrier();
>
> Why is this rcu_barrier() necessary?
I have read the rcubarrier.txt document and I got from that that an
rcu_barrier() is needed when unloading a module. But maybe I got it wrong.
So the idea after disabling the jump lables all pending readers in
task_cls_classid() have left. THe same thing is done in the old code
with the dynamic id part. With the difference that synchronize_rcu() is
used.
> In general, please explain what
> synchronization is going on when using sync constructs which aren't
> obvious - e.g. memory barriers, rcu barriers.
Sure, I will keep this in mind.
^ permalink raw reply
* Re: [PATCH v2 00/10] cgroup: Assign subsystem IDs during compile time
From: Daniel Wagner @ 2012-08-25 16:49 UTC (permalink / raw)
To: Tejun Heo
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
David S. Miller, Andrew Morton, Eric Dumazet, Gao feng,
Glauber Costa, Jamal Hadi Salim, John Fastabend,
Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <20120824231528.GO21325-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Hi Tejun,
On 25.08.2012 01:15, Tejun Heo wrote:
> Hello, Daniel.
>
> On Fri, Aug 24, 2012 at 04:01:34PM +0200, Daniel Wagner wrote:
>> CGROUP_BUILTIN_SUBSYS_COUNT is also gone in this version. This time I
>> trade space for speed. Some extra cycles are spend to identify the
>> modules in the for loops, e.g.
>
> There's no point in optimizing either space or speed here. It's not a
> hot path by any stretch of imagination. Please focus on making it
> simple.
Will do. Thanks a lot for your feedback. I'll update the series accordingly.
cheers,
daniel
^ permalink raw reply
* Re: [PATCH] af_unix: fix unix_nr_socks check in unix_create1()
From: Xi Wang @ 2012-08-25 16:17 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David S. Miller, Eric Dumazet
In-Reply-To: <1345876989.19483.83.camel@edumazet-glaptop>
On Aug 25, 2012, at 2:43 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> I dont think this patch is right.
>
> atomic_long_inc_return() is expensive, more than atomic_long_inc()
>
> And setting a 2^63 limit for max number of files is plain wrong,
> as there is no way a kernel can allocate 2^63 files structures.
>
> (same apply on 32bit arches, but for 2^31)
>
> If you feel you must warn a sysadmin of stupid settings, add sane
> boundaries in kernel/sysctl.c, and send your patch to lkml instead.
Yeah, I agree it's better to limit max_files in sysctl. I will send
another patch instead. Thanks.
- xi
^ permalink raw reply
* [PATCH RFC v2 0/2] Interface for TCP Metrics
From: Julian Anastasov @ 2012-08-25 15:00 UTC (permalink / raw)
To: netdev
This patchset contains 2 patches, one for kernel
and one for iproute2. We add DUMP/GET/DEL support for
genl "tcp_metrics" and minimal support for filtering
by address prefix and family in user space.
I tested show/del/flush, filtering by family, IPv4 prefix,
output for metrics such as rtt, rttvar, cwnd, ssthresh (after
modifying route). I didn't tested output for fast open.
May be some corrections in output can be desired.
The kernel patch has some parts to check:
- in tcp_metrics_nl_cmd_del I'm trying to flush all addresses
with single request, eg. when no family, address or other
selectors are provided. I use some arbitrary counter sync_count
to force memory to be freed in parts by using synchronize_rcu,
note that we are under genl_mutex, so may be this is bad idea
to delay other genl users? My idea was to avoid many commands
on "flush all", may be we should trigger flush by using some
other mechanism that is pernet? And I don't know how to test
it properly without large cache.
- I enabled lockdep in config and don't see any warnings.
make C=2 net/ipv4/tcp_metrics.o is silent too.
v2:
- patch 1: remove rcu_assign_pointer, add tcp_metrics_flush_all
- patch 2: properly flush by specifying address, improve man page
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox