[PATCH 0/4] ipmi: add limits on users and messages

[PATCH 0/4] ipmi: add limits on users and messages

[minyard]

I had actually already started working on a set of patches for this, but
I've incorporated some of your work.

There were problems with your patches:

* The limits on messages were global, not per-user.  This could cause
  unfairness in the interface.

* The counts were on the BMC, not on the interface.  The interface is
  the right place to put them, as that's where the messages flow
  through, and it's easier to do.

* Going through all the messages to get the count is kind of inefficient
  to do on a per-send basis.  Keep a count instead.

* The ability to flush messages is a no-go.  The IPMI driver guarantees
  responses and internal kernel users (and external users) rely on that
  property.  A flush could break the watchdog or ACPI.  The messages
  will just have to time out.

This is my proposal for your review.

Thanks,

-corey

[PATCH 1/4] ipmi: Add a limit on the number of users that may use IPMI

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Each user uses memory, we need limits to avoid a rogue program from
running the system out of memory.

Based on work by Chen Guanqiao <chen.chenchacha@foxmail.com>

Cc: Chen Guanqiao <chen.chenchacha@foxmail.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/char/ipmi/ipmi_msghandler.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index c59265146e9c..de80bf4c4e4c 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -145,6 +145,12 @@ module_param(default_max_retries, uint, 0644);
 MODULE_PARM_DESC(default_max_retries,
 		 "The time (milliseconds) between retry sends in maintenance mode");
 
+/* The default maximum number of users that may register. */
+static unsigned int max_users = 30;
+module_param(max_users, uint, 0644);
+MODULE_PARM_DESC(max_users,
+		 "The most users that may use the IPMI stack at one time.");
+
 /* Call every ~1000 ms. */
 #define IPMI_TIMEOUT_TIME	1000
 
@@ -442,6 +448,7 @@ struct ipmi_smi {
 	 */
 	struct list_head users;
 	struct srcu_struct users_srcu;
+	atomic_t nr_users;
 
 	/* Used for wake ups at startup. */
 	wait_queue_head_t waitq;
@@ -1230,6 +1237,11 @@ int ipmi_create_user(unsigned int          if_num,
 	goto out_kfree;
 
  found:
+	if (atomic_add_return(1, &intf->nr_users) > max_users) {
+		rv = -EBUSY;
+		goto out_kfree;
+	}
+
 	INIT_WORK(&new_user->remove_work, free_user_work);
 
 	rv = init_srcu_struct(&new_user->release_barrier);
@@ -1262,6 +1274,7 @@ int ipmi_create_user(unsigned int          if_num,
 	return 0;
 
 out_kfree:
+	atomic_sub(1, &intf->nr_users);
 	srcu_read_unlock(&ipmi_interfaces_srcu, index);
 	vfree(new_user);
 	return rv;
@@ -1336,6 +1349,7 @@ static void _ipmi_destroy_user(struct ipmi_user *user)
 	/* Remove the user from the interface's sequence table. */
 	spin_lock_irqsave(&intf->seq_lock, flags);
 	list_del_rcu(&user->link);
+	atomic_dec(&intf->nr_users);
 
 	for (i = 0; i < IPMI_IPMB_NUM_SEQ; i++) {
 		if (intf->seq_table[i].inuse
@@ -3529,6 +3543,7 @@ int ipmi_add_smi(struct module         *owner,
 	if (slave_addr != 0)
 		intf->addrinfo[0].address = slave_addr;
 	INIT_LIST_HEAD(&intf->users);
+	atomic_set(&intf->nr_users, 0);
 	intf->handlers = handlers;
 	intf->send_info = send_info;
 	spin_lock_init(&intf->seq_lock);
-- 
2.25.1

[PATCH 2/4] ipmi: Limit the number of message a user may have outstanding

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This way a rogue application can't use up a bunch of memory.

Based on work by Chen Guanqiao <chen.chenchacha@foxmail.com>

Cc: Chen Guanqiao <chen.chenchacha@foxmail.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/char/ipmi/ipmi_msghandler.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index de80bf4c4e4c..2a05199e8224 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -151,6 +151,12 @@ module_param(max_users, uint, 0644);
 MODULE_PARM_DESC(max_users,
 		 "The most users that may use the IPMI stack at one time.");
 
+/* The default maximum number of message a user may have outstanding. */
+static unsigned int max_msgs_per_user = 100;
+module_param(max_msgs_per_user, uint, 0644);
+MODULE_PARM_DESC(max_msgs_per_user,
+		 "The most message a user may have outstanding.");
+
 /* Call every ~1000 ms. */
 #define IPMI_TIMEOUT_TIME	1000
 
@@ -193,6 +199,8 @@ struct ipmi_user {
 	/* Does this interface receive IPMI events? */
 	bool gets_events;
 
+	atomic_t nr_msgs;
+
 	/* Free must run in process context for RCU cleanup. */
 	struct work_struct remove_work;
 };
@@ -934,11 +942,13 @@ static int deliver_response(struct ipmi_smi *intf, struct ipmi_recv_msg *msg)
 		 * risk.  At this moment, simply skip it in that case.
 		 */
 		ipmi_free_recv_msg(msg);
+		atomic_dec(&msg->user->nr_msgs);
 	} else {
 		int index;
 		struct ipmi_user *user = acquire_ipmi_user(msg->user, &index);
 
 		if (user) {
+			atomic_dec(&user->nr_msgs);
 			user->handler->ipmi_recv_hndl(msg, user->handler_data);
 			release_ipmi_user(user, index);
 		} else {
@@ -1256,6 +1266,7 @@ int ipmi_create_user(unsigned int          if_num,
 	/* Note that each existing user holds a refcount to the interface. */
 	kref_get(&intf->refcount);
 
+	atomic_set(&new_user->nr_msgs, 0);
 	kref_init(&new_user->refcount);
 	new_user->handler = handler;
 	new_user->handler_data = handler_data;
@@ -2298,6 +2309,14 @@ static int i_ipmi_request(struct ipmi_user     *user,
 	struct ipmi_recv_msg *recv_msg;
 	int rv = 0;
 
+	if (user) {
+		if (atomic_add_return(1, &user->nr_msgs) > max_msgs_per_user) {
+			atomic_dec(&user->nr_msgs);
+			rv = -EBUSY;
+			goto out;
+		}
+	}
+
 	if (supplied_recv)
 		recv_msg = supplied_recv;
 	else {
@@ -2369,6 +2388,8 @@ static int i_ipmi_request(struct ipmi_user     *user,
 	rcu_read_unlock();
 
 out:
+	if (rv && user)
+		atomic_dec(&user->nr_msgs);
 	return rv;
 }
 
-- 
2.25.1

Re: [PATCH 2/4] ipmi: Limit the number of message a user may have outstanding

[chenchacha]

> @@ -2298,6 +2309,14 @@ static int i_ipmi_request(struct ipmi_user     *user,
>   	struct ipmi_recv_msg *recv_msg;
>   	int rv = 0;
>   
> +	if (user) {
> +		if (atomic_add_return(1, &user->nr_msgs) > max_msgs_per_user) {
> +			atomic_dec(&user->nr_msgs);
> +			rv = -EBUSY;
> +			goto out;
> +		}
> +	}
> +
> @@ -2369,6 +2388,8 @@ static int i_ipmi_request(struct ipmi_user     *user,
>   	rcu_read_unlock();
>   
>   out:
> +	if (rv && user)
> +		atomic_dec(&user->nr_msgs);
>   	return rv;
>   }

If the number of msg is greater than the limit, the nr_msgs will be 
decrease twice.

Should it be returned directory?

--
Chen Guanqiao

Re: [PATCH 2/4] ipmi: Limit the number of message a user may have outstanding

[Corey Minyard]

On Wed, Mar 30, 2022 at 10:44:50PM +0800, chenchacha wrote:
> > @@ -2298,6 +2309,14 @@ static int i_ipmi_request(struct ipmi_user     *user,
> >   	struct ipmi_recv_msg *recv_msg;
> >   	int rv = 0;
> > +	if (user) {
> > +		if (atomic_add_return(1, &user->nr_msgs) > max_msgs_per_user) {
> > +			atomic_dec(&user->nr_msgs);
> > +			rv = -EBUSY;
> > +			goto out;
> > +		}
> > +	}
> > +
> > @@ -2369,6 +2388,8 @@ static int i_ipmi_request(struct ipmi_user     *user,
> >   	rcu_read_unlock();
> >   out:
> > +	if (rv && user)
> > +		atomic_dec(&user->nr_msgs);
> >   	return rv;
> >   }
> 
> If the number of msg is greater than the limit, the nr_msgs will be decrease
> twice.
> 
> Should it be returned directory?

Oh wait, yeah, I screwed that up.  I added the first decrement later,
after I "noticed" it was missing.  I should add a comment there.
Thanks.

-corey

> 
> --
> Chen Guanqiao

[PATCH 3/4] ipmi: Add a sysfs interface to view the number of users

[minyard]

From: Corey Minyard <cminyard@mvista.com>

A count of users is kept for each interface, allow it to be viewed.

Based on work by Chen Guanqiao <chen.chenchacha@foxmail.com>

Cc: Chen Guanqiao <chen.chenchacha@foxmail.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/char/ipmi/ipmi_msghandler.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 2a05199e8224..3ba188e05ca6 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -457,6 +457,8 @@ struct ipmi_smi {
 	struct list_head users;
 	struct srcu_struct users_srcu;
 	atomic_t nr_users;
+	struct device_attribute nr_users_devattr;
+
 
 	/* Used for wake ups at startup. */
 	wait_queue_head_t waitq;
@@ -3506,6 +3508,17 @@ void ipmi_poll_interface(struct ipmi_user *user)
 }
 EXPORT_SYMBOL(ipmi_poll_interface);
 
+static ssize_t nr_users_show(struct device *dev,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	struct ipmi_smi *intf = container_of(attr,
+			 struct ipmi_smi, nr_users_devattr);
+
+	return sysfs_emit(buf, "%d\n", atomic_read(&intf->nr_users));
+}
+static DEVICE_ATTR_RO(nr_users);
+
 static void redo_bmc_reg(struct work_struct *work)
 {
 	struct ipmi_smi *intf = container_of(work, struct ipmi_smi,
@@ -3628,6 +3641,12 @@ int ipmi_add_smi(struct module         *owner,
 	if (rv)
 		goto out_err_bmc_reg;
 
+	intf->nr_users_devattr = dev_attr_nr_users;
+	sysfs_attr_init(&intf->nr_users_devattr.attr);
+	rv = device_create_file(intf->si_dev, &intf->nr_users_devattr);
+	if (rv)
+		goto out_err_bmc_reg;
+
 	/*
 	 * Keep memory order straight for RCU readers.  Make
 	 * sure everything else is committed to memory before
@@ -3724,6 +3743,8 @@ void ipmi_unregister_smi(struct ipmi_smi *intf)
 
 	/* At this point no users can be added to the interface. */
 
+	device_remove_file(intf->si_dev, &intf->nr_users_devattr);
+
 	/*
 	 * Call all the watcher interfaces to tell them that
 	 * an interface is going away.
-- 
2.25.1

[PATCH 4/4] ipmi: Add a sysfs count of total outstanding messages for an interface

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Go through each user and add its message count to a total and print the
total.

It would be nice to have a per-user file, but there's no user sysfs
entity at this point to hang it off of.  Probably not worth the effort.

Based on work by Chen Guanqiao <chen.chenchacha@foxmail.com>

Cc: Chen Guanqiao <chen.chenchacha@foxmail.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/char/ipmi/ipmi_msghandler.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 3ba188e05ca6..e30223763a2a 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -458,6 +458,7 @@ struct ipmi_smi {
 	struct srcu_struct users_srcu;
 	atomic_t nr_users;
 	struct device_attribute nr_users_devattr;
+	struct device_attribute nr_msgs_devattr;
 
 
 	/* Used for wake ups at startup. */
@@ -3519,6 +3520,25 @@ static ssize_t nr_users_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(nr_users);
 
+static ssize_t nr_msgs_show(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct ipmi_smi *intf = container_of(attr,
+			 struct ipmi_smi, nr_msgs_devattr);
+	struct ipmi_user *user;
+	int index;
+	unsigned int count = 0;
+
+	index = srcu_read_lock(&intf->users_srcu);
+	list_for_each_entry_rcu(user, &intf->users, link)
+		count += atomic_read(&user->nr_msgs);
+	srcu_read_unlock(&intf->users_srcu, index);
+
+	return sysfs_emit(buf, "%u\n", count);
+}
+static DEVICE_ATTR_RO(nr_msgs);
+
 static void redo_bmc_reg(struct work_struct *work)
 {
 	struct ipmi_smi *intf = container_of(work, struct ipmi_smi,
@@ -3647,6 +3667,14 @@ int ipmi_add_smi(struct module         *owner,
 	if (rv)
 		goto out_err_bmc_reg;
 
+	intf->nr_msgs_devattr = dev_attr_nr_msgs;
+	sysfs_attr_init(&intf->nr_msgs_devattr.attr);
+	rv = device_create_file(intf->si_dev, &intf->nr_msgs_devattr);
+	if (rv) {
+		device_remove_file(intf->si_dev, &intf->nr_users_devattr);
+		goto out_err_bmc_reg;
+	}
+
 	/*
 	 * Keep memory order straight for RCU readers.  Make
 	 * sure everything else is committed to memory before
@@ -3743,6 +3771,7 @@ void ipmi_unregister_smi(struct ipmi_smi *intf)
 
 	/* At this point no users can be added to the interface. */
 
+	device_remove_file(intf->si_dev, &intf->nr_msgs_devattr);
 	device_remove_file(intf->si_dev, &intf->nr_users_devattr);
 
 	/*
-- 
2.25.1

Re: [PATCH 0/4] ipmi: add limits on users and messages

[chenchacha]

On 2022/3/30 02:33, minyard@acm.org wrote:
> I had actually already started working on a set of patches for this, but
> I've incorporated some of your work.
> 
> There were problems with your patches:
> 
> * The limits on messages were global, not per-user.  This could cause
>    unfairness in the interface.
> 
> * The counts were on the BMC, not on the interface.  The interface is
>    the right place to put them, as that's where the messages flow
>    through, and it's easier to do.
> 
> * Going through all the messages to get the count is kind of inefficient
>    to do on a per-send basis.  Keep a count instead.
> 
> * The ability to flush messages is a no-go.  The IPMI driver guarantees
>    responses and internal kernel users (and external users) rely on that
>    property.  A flush could break the watchdog or ACPI.  The messages
>    will just have to time out.
> 
> This is my proposal for your review.
> 
> Thanks,
> 
> -corey

Hi Corey:

Thanks, My intention is to provide a debug method, it is not perfect.

I try to use your patch in cluster directly.

If the problem no longer occurs, there is no need to cleanup the messages.

--
Chen Guanqiao