On Tue, 1 Feb 2011, Jozsef Kadlecsik wrote:
> On Tue, 1 Feb 2011, Patrick McHardy wrote:
>
> > Am 31.01.2011 23:52, schrieb Jozsef Kadlecsik:
> > > +static int
> > > +call_ad(struct sk_buff *skb, struct ip_set *set,
> > > + struct nlattr *tb[], enum ipset_adt adt,
> > > + u32 flags, bool use_lineno)
> > > +{
> > > + int ret, retried = 0;
> > > + u32 lineno = 0;
> > > + bool eexist = flags & IPSET_FLAG_EXIST;
> > > +
> > > + do {
> > > + write_lock_bh(&set->lock);
> > > + ret = set->variant->uadt(set, tb, adt, &lineno, flags);
> > > + write_unlock_bh(&set->lock);
> > > + } while (ret == -EAGAIN &&
> > > + set->variant->resize &&
> > > + (ret = set->variant->resize(set, retried++)) == 0);
> > > +
> > > + if (!ret || (ret == -IPSET_ERR_EXIST && eexist))
> > > + return 0;
> > > + if (lineno && use_lineno) {
> > > + /* Error in restore/batch mode: send back lineno */
> > > + struct nlmsghdr *nlh = nlmsg_hdr(skb);
> > > + int min_len = NLMSG_SPACE(sizeof(struct nfgenmsg));
> > > + struct nlattr *cda[IPSET_ATTR_CMD_MAX+1];
> > > + struct nlattr *cmdattr = (void *)nlh + min_len;
> > > + u32 *errline;
> > > +
> > > + nla_parse(cda, IPSET_ATTR_CMD_MAX,
> > > + cmdattr, nlh->nlmsg_len - min_len,
> > > + ip_set_adt_policy);
> > > +
> > > + errline = nla_data(cda[IPSET_ATTR_LINENO]);
> > > +
> > > + *errline = lineno;
> >
> > This is still not correct. I didn't mean to remove the const attributes
> > (the message is still considered const by the higher layers, the netlink
> > functions just cast this away). You're modifying the received message,
> > I don't see how this can be useful to userspace.
>
> I can't find where the message is considered const in netlink/nfnetlink.
> It seems to be freely writable via skb.
>
> > I guess you're relying on that the original message is appended to a
> > nlmsgerr message. That doesn't seem right though, if you want to return
> > something to userspace, you should construct a new message.
>
> The message we are processing here carried multiple commands (each having
> an attribute with the line number of the given command) and one failed
> from some reason. We have to notify the userspace which command, at what
> line failed. For this reason the multi-command messages have got an
> attribute, which can be filled out with the line number - that happens
> here. The attribute is already there, the message is not enlarged, just
> the empty value is overwritten with the proper value.
>
> The line number reporting works this way, tested in the testsuite too.
>
> If I had to construct a completely new message and sent it, that'd be more
> or less the duplication of netlink_ack. Additionally I had to suppress
> netlink from sending an errmsg/ack too.
Hm, if I lie -EINTR to netlink, then I can construct and send the error
message manually and keep NLM_F_ACK at the same time. What do you think?
Please have a look at the attached patch.
Best regards,
Jozsef
-
E-mail : kadlec@xxxxxxxxxxxxxxxxx, kadlec@xxxxxxxxxxxx
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
H-1525 Budapest 114, POB. 49, Hungarydiff --git a/kernel/ip_set_core.c b/kernel/ip_set_core.c
index 19158bf..cc67d92 100644
--- a/kernel/ip_set_core.c
+++ b/kernel/ip_set_core.c
@@ -1103,7 +1103,7 @@ static const struct nla_policy ip_set_adt_policy[IPSET_ATTR_CMD_MAX + 1] = {
};
static int
-call_ad(struct sk_buff *skb, struct ip_set *set,
+call_ad(struct sock *ctnl, struct sk_buff *skb, struct ip_set *set,
struct nlattr *tb[], enum ipset_adt adt,
u32 flags, bool use_lineno)
{
@@ -1123,12 +1123,25 @@ call_ad(struct sk_buff *skb, struct ip_set *set,
return 0;
if (lineno && use_lineno) {
/* Error in restore/batch mode: send back lineno */
- struct nlmsghdr *nlh = nlmsg_hdr(skb);
+ struct nlmsghdr *rep, *nlh = nlmsg_hdr(skb);
+ struct sk_buff *skb2;
+ struct nlmsgerr *errmsg;
+ size_t payload = sizeof(*errmsg) + nlmsg_len(nlh);
int min_len = NLMSG_SPACE(sizeof(struct nfgenmsg));
struct nlattr *cda[IPSET_ATTR_CMD_MAX+1];
- struct nlattr *cmdattr = (void *)nlh + min_len;
+ struct nlattr *cmdattr;
u32 *errline;
+ skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (skb2 == NULL)
+ return -ENOMEM;
+ rep = __nlmsg_put(skb2, NETLINK_CB(skb).pid,
+ nlh->nlmsg_seq, NLMSG_ERROR, payload, 0);
+ errmsg = nlmsg_data(rep);
+ errmsg->error = ret;
+ memcpy(&errmsg->msg, nlh, nlh->nlmsg_len);
+ cmdattr = (void *)&errmsg->msg + min_len;
+
nla_parse(cda, IPSET_ATTR_CMD_MAX,
cmdattr, nlh->nlmsg_len - min_len,
ip_set_adt_policy);
@@ -1136,6 +1149,9 @@ call_ad(struct sk_buff *skb, struct ip_set *set,
errline = nla_data(cda[IPSET_ATTR_LINENO]);
*errline = lineno;
+
+ netlink_unicast(ctnl, skb2, NETLINK_CB(skb).pid, MSG_DONTWAIT);
+ return -EINTR;
}
return ret;
@@ -1174,7 +1190,8 @@ ip_set_uadd(struct sock *ctnl, struct sk_buff *skb,
attr[IPSET_ATTR_DATA],
set->type->adt_policy))
return -IPSET_ERR_PROTOCOL;
- ret = call_ad(skb, set, tb, IPSET_ADD, flags, use_lineno);
+ ret = call_ad(ctnl, skb, set, tb, IPSET_ADD, flags,
+ use_lineno);
} else {
int nla_rem;
@@ -1185,7 +1202,7 @@ ip_set_uadd(struct sock *ctnl, struct sk_buff *skb,
nla_parse_nested(tb, IPSET_ATTR_ADT_MAX, nla,
set->type->adt_policy))
return -IPSET_ERR_PROTOCOL;
- ret = call_ad(skb, set, tb, IPSET_ADD,
+ ret = call_ad(ctnl, skb, set, tb, IPSET_ADD,
flags, use_lineno);
if (ret < 0)
return ret;
@@ -1227,7 +1244,8 @@ ip_set_udel(struct sock *ctnl, struct sk_buff *skb,
attr[IPSET_ATTR_DATA],
set->type->adt_policy))
return -IPSET_ERR_PROTOCOL;
- ret = call_ad(skb, set, tb, IPSET_DEL, flags, use_lineno);
+ ret = call_ad(ctnl, skb, set, tb, IPSET_DEL, flags,
+ use_lineno);
} else {
int nla_rem;
@@ -1238,7 +1256,7 @@ ip_set_udel(struct sock *ctnl, struct sk_buff *skb,
nla_parse_nested(tb, IPSET_ATTR_ADT_MAX, nla,
set->type->adt_policy))
return -IPSET_ERR_PROTOCOL;
- ret = call_ad(skb, set, tb, IPSET_DEL,
+ ret = call_ad(ctnl, skb, set, tb, IPSET_DEL,
flags, use_lineno);
if (ret < 0)
return ret;
